From 7c5b2cbba9ce1e480c973904a751a9b2dcbae9ae Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Wed, 11 Nov 2026 16:25:58 -0500 Subject: [PATCH 1/4] Check permission earlier in set_rfd_attr; bound AsciiDoc reference expansion --- rfd-api/src/endpoints/rfd.rs | 15 ++++++++- rfd-data/src/content/asciidoc.rs | 56 +++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/rfd-api/src/endpoints/rfd.rs b/rfd-api/src/endpoints/rfd.rs index 7072c611..ca4aae17 100644 --- a/rfd-api/src/endpoints/rfd.rs +++ b/rfd-api/src/endpoints/rfd.rs @@ -19,7 +19,10 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use trace_request::trace_request; use tracing::instrument; -use v_api::{response::not_found, ApiContext}; +use v_api::{ + response::{forbidden, not_found}, + ApiContext, +}; use v_model::{permissions::Caller, storage::ListPagination}; use crate::{ @@ -749,6 +752,16 @@ async fn set_rfd_attr_op( body: &RfdAttrValue, ) -> Result, HttpError> { if let Ok(rfd_number) = number.parse::() { + if !caller.any( + [ + RfdPermission::UpdateRfd(rfd_number), + RfdPermission::UpdateRfdsAll, + ] + .iter(), + ) { + return Err(forbidden()); + } + // Get the latest revision let revision = ctx.view_rfd_revision(caller, rfd_number, None).await?; diff --git a/rfd-data/src/content/asciidoc.rs b/rfd-data/src/content/asciidoc.rs index 65cb6310..e134cccb 100644 --- a/rfd-data/src/content/asciidoc.rs +++ b/rfd-data/src/content/asciidoc.rs @@ -2,12 +2,15 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use regex::{regex, Regex}; +use regex::{regex, NoExpand, Regex}; use std::borrow::Cow; use super::RfdDocument; use crate::SingleLineString; +/// Extra bytes reference resolution may add beyond 2x the input size +const MAX_REFERENCE_EXPANSION_ALLOWANCE: usize = 64 * 1024; + /// The text data of an Asciidoc RFD #[derive(Debug, Clone)] pub struct RfdAsciidoc<'a> { @@ -33,15 +36,32 @@ impl<'a> RfdAsciidoc<'a> { if let Some(captures) = pattern.captures(content) { let prefix = captures.get(1).map(|c| c.as_str()).unwrap_or_default(); + let replacement = format!( + "{}{}", + prefix, + Self::attr("authors", content).unwrap_or_default() + ); + + // Bound expansion size; leave oversized documents unresolved + let max_len = content + .len() + .saturating_mul(2) + .saturating_add(MAX_REFERENCE_EXPANSION_ALLOWANCE); + let expanded_len = pattern + .find_iter(content) + .try_fold(content.len(), |len, _| len.checked_add(replacement.len())); + + if !expanded_len.is_some_and(|len| len <= max_len) { + tracing::warn!( + content_len = content.len(), + "Skipping reference resolution that would exceed the maximum document size" + ); + return content.to_string(); + } + + // NoExpand: don't treat `$` as regex capture reference ($1, etc) pattern - .replace_all( - content, - format!( - "{}{}", - prefix, - Self::attr("authors", content).unwrap_or_default() - ), - ) + .replace_all(content, NoExpand(&replacement)) .to_string() } else { content.to_string() @@ -1052,4 +1072,22 @@ include::sub_doc2.adoc[]"#; include.perform_replacement(original, replacement_content) ); } + + #[test] + fn test_skip_unbounded_reference_expansion() { + let authors = "a".repeat(100_000); + let references = "{authors}\n".repeat(10_000); + let content = format!(":authors: {}\n\n= RFD 123 Place\n\n{}", authors, references); + + let rfd = RfdAsciidoc::new(content.as_str()); + assert!(rfd.resolved.len() <= 2 * content.len() + MAX_REFERENCE_EXPANSION_ALLOWANCE); + assert!(rfd.resolved.contains("{authors}")); + } + + #[test] + fn test_reference_value_inserted_literally() { + let content = ":authors: Cost $1 ${b} $$\n\n= RFD 123 Place\n\nBy {authors}\n"; + let rfd = RfdAsciidoc::new(content); + assert!(rfd.resolved.contains("By Cost $1 ${b} $$\n")); + } } From f018b4396a95bf618bb30caff76bbb3ebc312830 Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Thu, 12 Nov 2026 09:17:31 -0500 Subject: [PATCH 2/4] Only expand first {authors} reference. --- rfd-data/src/content/asciidoc.rs | 29 +++++------------------------ 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/rfd-data/src/content/asciidoc.rs b/rfd-data/src/content/asciidoc.rs index e134cccb..aaeedd99 100644 --- a/rfd-data/src/content/asciidoc.rs +++ b/rfd-data/src/content/asciidoc.rs @@ -8,9 +8,6 @@ use std::borrow::Cow; use super::RfdDocument; use crate::SingleLineString; -/// Extra bytes reference resolution may add beyond 2x the input size -const MAX_REFERENCE_EXPANSION_ALLOWANCE: usize = 64 * 1024; - /// The text data of an Asciidoc RFD #[derive(Debug, Clone)] pub struct RfdAsciidoc<'a> { @@ -31,6 +28,7 @@ impl<'a> RfdAsciidoc<'a> { } /// Resolve attribute references within the document. Currently only `{authors}` is supported. + /// Only the first reference is expanded, which bounds growth to the size of one value. fn resolve_references(content: &str) -> String { let pattern = regex!(r"(?[^\\]?)(\{authors\})"); @@ -42,26 +40,9 @@ impl<'a> RfdAsciidoc<'a> { Self::attr("authors", content).unwrap_or_default() ); - // Bound expansion size; leave oversized documents unresolved - let max_len = content - .len() - .saturating_mul(2) - .saturating_add(MAX_REFERENCE_EXPANSION_ALLOWANCE); - let expanded_len = pattern - .find_iter(content) - .try_fold(content.len(), |len, _| len.checked_add(replacement.len())); - - if !expanded_len.is_some_and(|len| len <= max_len) { - tracing::warn!( - content_len = content.len(), - "Skipping reference resolution that would exceed the maximum document size" - ); - return content.to_string(); - } - // NoExpand: don't treat `$` as regex capture reference ($1, etc) pattern - .replace_all(content, NoExpand(&replacement)) + .replacen(content, 1, NoExpand(&replacement)) .to_string() } else { content.to_string() @@ -1074,14 +1055,14 @@ include::sub_doc2.adoc[]"#; } #[test] - fn test_skip_unbounded_reference_expansion() { + fn test_only_first_reference_expanded() { let authors = "a".repeat(100_000); let references = "{authors}\n".repeat(10_000); let content = format!(":authors: {}\n\n= RFD 123 Place\n\n{}", authors, references); let rfd = RfdAsciidoc::new(content.as_str()); - assert!(rfd.resolved.len() <= 2 * content.len() + MAX_REFERENCE_EXPANSION_ALLOWANCE); - assert!(rfd.resolved.contains("{authors}")); + assert!(rfd.resolved.len() <= content.len() + authors.len()); + assert_eq!(rfd.resolved.matches("{authors}").count(), 10_000 - 1); } #[test] From 8ce94d922423f1611415afcdfd815cd71329314b Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Thu, 12 Nov 2026 09:56:58 -0500 Subject: [PATCH 3/4] Move set_rfd_attr logic into RfdContext::update_rfd_attr The permission check now lives in the context and wraps the update, instead of an early return in the endpoint. The latest revision is fetched once and used for both the edited content and the base commit. --- rfd-api/src/context.rs | 54 +++++++++++++++++++++++++++++++++- rfd-api/src/endpoints/rfd.rs | 56 +++++++----------------------------- 2 files changed, 64 insertions(+), 46 deletions(-) diff --git a/rfd-api/src/context.rs b/rfd-api/src/context.rs index 6636e42c..3645fa38 100644 --- a/rfd-api/src/context.rs +++ b/rfd-api/src/context.rs @@ -12,7 +12,7 @@ use octorust::{ use partial_struct::partial; use rfd_data::{ content::{RfdContent, RfdDocument, RfdTemplate, TemplateError}, - RfdNumber, + RfdNumber, RfdState, SingleLineString, }; use rfd_github::{GitHubError, GitHubNewRfdNumber, GitHubRfdRepo}; use rfd_model::{ @@ -81,6 +81,14 @@ pub struct ContentContext { pub new_template: RfdTemplate, } +/// A single attribute change to apply to the header of a RFD document +#[derive(Debug)] +pub enum RfdAttrUpdate { + Discussion(SingleLineString), + Labels(SingleLineString), + State(RfdState), +} + #[derive(Debug, Error)] pub enum UpdateRfdContentError { #[error(transparent)] @@ -691,6 +699,50 @@ impl RfdContext { } } + #[allow(clippy::result_large_err)] + #[instrument(skip(self, caller))] + pub async fn update_rfd_attr( + &self, + caller: &Caller, + rfd_number: i32, + update: RfdAttrUpdate, + message: Option<&str>, + ) -> ResourceResult, UpdateRfdContentError> { + if caller.any( + [ + RfdPermission::UpdateRfd(rfd_number), + RfdPermission::UpdateRfdsAll, + ] + .iter(), + ) { + let latest_revision = self + .get_latest_rfd_revision(caller, rfd_number) + .await + .inner_err_into()?; + + let sha = latest_revision.commit.clone(); + let mut content: RfdContent = latest_revision.into(); + + match update { + RfdAttrUpdate::Discussion(value) => content.update_discussion(&value), + RfdAttrUpdate::Labels(value) => content.update_labels(&value), + RfdAttrUpdate::State(state) => content.update_state(&state.into()), + }; + + tracing::info!("Updated attribute in RFD document"); + + // Persist the data back to GitHub. Note that we do not store this back to the database. + // We rely on GitHub as the source of truth and revisions are required to be linked to + // commits + self.commit_rfd_document(caller, rfd_number.into(), content.raw(), message, sha, None) + .await?; + + Ok(content) + } else { + resource_restricted() + } + } + #[allow(clippy::result_large_err)] #[instrument(skip(self, caller, document))] pub async fn update_rfd_document( diff --git a/rfd-api/src/endpoints/rfd.rs b/rfd-api/src/endpoints/rfd.rs index ca4aae17..a9e4aac4 100644 --- a/rfd-api/src/endpoints/rfd.rs +++ b/rfd-api/src/endpoints/rfd.rs @@ -19,17 +19,14 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use trace_request::trace_request; use tracing::instrument; -use v_api::{ - response::{forbidden, not_found}, - ApiContext, -}; +use v_api::{response::not_found, ApiContext}; use v_model::{permissions::Caller, storage::ListPagination}; use crate::{ caller::CallerExt, context::{ - RfdContext, RfdRevisionIdentifier, RfdRevisionMeta, RfdRevisionMetadataChange, RfdWithPdf, - RfdWithRaw, RfdWithoutContent, + RfdAttrUpdate, RfdContext, RfdRevisionIdentifier, RfdRevisionMeta, + RfdRevisionMetadataChange, RfdWithPdf, RfdWithRaw, RfdWithoutContent, }, endpoints::UNLIMITED, permissions::RfdPermission, @@ -752,51 +749,20 @@ async fn set_rfd_attr_op( body: &RfdAttrValue, ) -> Result, HttpError> { if let Ok(rfd_number) = number.parse::() { - if !caller.any( - [ - RfdPermission::UpdateRfd(rfd_number), - RfdPermission::UpdateRfdsAll, - ] - .iter(), - ) { - return Err(forbidden()); - } - - // Get the latest revision - let revision = ctx.view_rfd_revision(caller, rfd_number, None).await?; - - // TODO: Get rid of these clones - let mut content = match revision.content_format { - ContentFormat::Asciidoc => RfdContent::Asciidoc(RfdAsciidoc::new(revision.content)), - ContentFormat::Markdown => RfdContent::Markdown(RfdMarkdown::new(revision.content)), - }; - - // Update the requested attribute - match &attr { - RfdAttrName::Discussion => content.update_discussion(&body.value), - RfdAttrName::Labels => content.update_labels(&body.value), + let update = match &attr { + RfdAttrName::Discussion => RfdAttrUpdate::Discussion(body.value.clone()), + RfdAttrName::Labels => RfdAttrUpdate::Labels(body.value.clone()), RfdAttrName::State => { - let state: RfdState = body.value.as_str().try_into().map_err(|err| { + RfdAttrUpdate::State(body.value.as_str().try_into().map_err(|err| { tracing::info!(?err, "Invalid state was supplied"); HttpError::for_bad_request(None, "Invalid RFD state".to_string()) - })?; - content.update_state(&state.into()) + })?) } }; - tracing::info!("Updated attribute in RFD document"); - - // Persist the data back to GitHub. Note that we do not store this back to the database. - // We rely on GitHub as the source of truth and revisions are required to tbe linked to - // commits - ctx.update_rfd_document( - caller, - rfd_number, - content.raw(), - body.message.as_deref(), - None, - ) - .await?; + let content = ctx + .update_rfd_attr(caller, rfd_number, update, body.message.as_deref()) + .await?; extract_attr(&attr, &content).map(HttpResponseAccepted) } else { From 160c4b4646c8ffb2898c8f8598715c345f01fef7 Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Thu, 12 Nov 2026 09:56:58 -0500 Subject: [PATCH 4/4] Drop test_only_first_reference_expanded --- rfd-data/src/content/asciidoc.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/rfd-data/src/content/asciidoc.rs b/rfd-data/src/content/asciidoc.rs index aaeedd99..f81f3640 100644 --- a/rfd-data/src/content/asciidoc.rs +++ b/rfd-data/src/content/asciidoc.rs @@ -1054,17 +1054,6 @@ include::sub_doc2.adoc[]"#; ); } - #[test] - fn test_only_first_reference_expanded() { - let authors = "a".repeat(100_000); - let references = "{authors}\n".repeat(10_000); - let content = format!(":authors: {}\n\n= RFD 123 Place\n\n{}", authors, references); - - let rfd = RfdAsciidoc::new(content.as_str()); - assert!(rfd.resolved.len() <= content.len() + authors.len()); - assert_eq!(rfd.resolved.matches("{authors}").count(), 10_000 - 1); - } - #[test] fn test_reference_value_inserted_literally() { let content = ":authors: Cost $1 ${b} $$\n\n= RFD 123 Place\n\nBy {authors}\n";