Skip to content

Commit d7b6b85

Browse files
boutellboutell
authored
Merge commit from fork
1 parent 87cccf4 commit d7b6b85

4 files changed

Lines changed: 317 additions & 60 deletions

File tree

‎.changeset/secure-form-file-uploads.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@apostrophecms/form": patch
3+
---
4+
5+
Security (GHSA-rgg4-476q-xgcg): file attachments are no longer stored before a form submission passes validation. Previously, when a submission was rejected — for example due to a failed reCAPTCHA challenge or a missing required field — any uploaded files had already been written as publicly accessible attachments, and those orphaned records were never reclaimed by garbage collection. Submissions are now rejected before any files are stored, and any attachments created while processing a submission that is ultimately rejected are removed. Thanks to [H3xV0rT3x](https://github.com/H3xV0rT3x) for reporting this issue.

‎claude-tools/run-form-tests.sh‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
#!/bin/bash
2+
# Run the @apostrophecms/form test suite against a chosen DB adapter and log
3+
# output to claude-tools/logs/form-<adapter>.log. Usage:
4+
#
5+
# ./claude-tools/run-form-tests.sh mongodb
6+
# ./claude-tools/run-form-tests.sh postgres
7+
# ./claude-tools/run-form-tests.sh sqlite
8+
# ./claude-tools/run-form-tests.sh mongodb "orphan" # only tests matching grep
9+
#
10+
# The optional second argument is passed to `mocha --grep` so you can run
11+
# just the tests you care about while iterating.
12+
#
13+
# NEVER run multiple adapters in parallel — the test suite is not designed
14+
# for concurrent runs and the host has limited resources.
15+
16+
set -u
17+
adapter="${1:-mongodb}"
18+
grep_filter="${2:-}"
19+
20+
root="$(cd "$(dirname "$0")/.." && pwd)"
21+
logdir="$root/claude-tools/logs"
22+
mkdir -p "$logdir"
23+
log="$logdir/form-$adapter.log"
24+
: > "$log"
25+
26+
echo "=== $adapter form tests ($(date -Is)) grep='${grep_filter}' ===" | tee -a "$log"
27+
28+
cd "$root/packages/form"
29+
30+
extra=()
31+
if [[ "$adapter" == "postgres" ]]; then
32+
extra=(env PGPASSWORD=testpassword)
33+
fi
34+
35+
mocha_args=(-t 25000)
36+
if [[ -n "$grep_filter" ]]; then
37+
mocha_args+=(--grep "$grep_filter")
38+
fi
39+
40+
APOS_TEST_DB_PROTOCOL="$adapter" "${extra[@]}" ../../node_modules/.bin/mocha "${mocha_args[@]}" >> "$log" 2>&1
41+
code=$?
42+
43+
echo "=== exit=$code ===" | tee -a "$log"
44+
45+
echo ""
46+
echo "----- FAILURES (if any) -----"
47+
# Mocha marks failing tests with a numbered list under "N passing/failing".
48+
# Print the failing test titles and the summary line for a quick report.
49+
grep -nE "passing|failing|pending" "$log" | tail -5
50+
echo "-----------------------------"
51+
# Extract the "N) test title" blocks that mocha prints for failures.
52+
awk '/^ [0-9]+\) /{flag=1} flag{print} /^$/{if(flag>0)flag++} flag>3{flag=0}' "$log" | head -60
53+
54+
exit "$code"

‎packages/form/lib/processor.js‎

Lines changed: 110 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -24,80 +24,109 @@ module.exports = function (self) {
2424
}
2525
}
2626

27-
// Find any file field submissions and insert the files as attachments
28-
for (const [ field, value ] of Object.entries(input)) {
29-
if (value === 'files-pending') {
30-
try {
31-
input[field] = await self.insertFieldFiles(req, field, req.files);
32-
} catch (error) {
33-
self.apos.util.error(error);
34-
formErrors.push({
35-
field,
36-
error: 'invalid',
37-
message: req.t('aposForm:fileUploadError')
38-
});
27+
// Reject invalid submissions (for example a failed reCAPTCHA challenge)
28+
// BEFORE storing any uploaded files. Otherwise a rejected submission
29+
// would still persist a publicly accessible attachment that garbage
30+
// collection never reclaims (GHSA-rgg4-476q-xgcg).
31+
if (formErrors.length > 0) {
32+
throw self.apos.error('invalid', {
33+
formErrors
34+
});
35+
}
36+
37+
// Field names for the query params check list. Declared here so it
38+
// remains available after the try block below.
39+
const fieldNames = [];
40+
41+
// Track attachments stored while processing this submission so they can
42+
// be removed if the submission is ultimately rejected. This ensures a
43+
// rejected submission never leaves persistent, publicly accessible files
44+
// behind (GHSA-rgg4-476q-xgcg).
45+
const insertedAttachmentIds = [];
46+
47+
try {
48+
// Find any file field submissions and insert the files as attachments
49+
for (const [ field, value ] of Object.entries(input)) {
50+
if (value === 'files-pending') {
51+
try {
52+
input[field] = await self.insertFieldFiles(req, field, req.files);
53+
insertedAttachmentIds.push(...input[field]);
54+
} catch (error) {
55+
self.apos.util.error(error);
56+
formErrors.push({
57+
field,
58+
error: 'invalid',
59+
message: req.t('aposForm:fileUploadError')
60+
});
61+
}
3962
}
4063
}
41-
}
4264

43-
// Recursively walk the area and its sub-areas so we find
44-
// fields nested in two-column widgets and the like
65+
// Recursively walk the area and its sub-areas so we find
66+
// fields nested in two-column widgets and the like
4567

46-
// walk is not an async function so build an array of them to start
47-
const areas = [];
68+
// walk is not an async function so build an array of them to start
69+
const areas = [];
4870

49-
self.apos.area.walk({
50-
contents: form.contents
51-
}, function(area) {
52-
areas.push(area);
53-
});
71+
self.apos.area.walk({
72+
contents: form.contents
73+
}, function(area) {
74+
areas.push(area);
75+
});
5476

55-
const fieldNames = [];
56-
const conditionals = {};
57-
const skipFields = [];
58-
59-
// Populate the conditionals object fully to clear disabled values
60-
// before starting sanitization.
61-
for (const area of areas) {
62-
const widgets = area.items || [];
63-
for (const widget of widgets) {
64-
// Capture field names for the params check list.
65-
fieldNames.push(widget.fieldName);
66-
67-
if (widget.type === '@apostrophecms/form-conditional') {
68-
self.trackConditionals(conditionals, widget);
77+
const conditionals = {};
78+
const skipFields = [];
79+
80+
// Populate the conditionals object fully to clear disabled values
81+
// before starting sanitization.
82+
for (const area of areas) {
83+
const widgets = area.items || [];
84+
for (const widget of widgets) {
85+
// Capture field names for the params check list.
86+
fieldNames.push(widget.fieldName);
87+
88+
if (widget.type === '@apostrophecms/form-conditional') {
89+
self.trackConditionals(conditionals, widget);
90+
}
6991
}
7092
}
71-
}
72-
73-
self.collectToSkip(input, conditionals, skipFields);
7493

75-
for (const area of areas) {
76-
const widgets = area.items || [];
77-
for (const widget of widgets) {
78-
const manager = self.apos.area.getWidgetManager(widget.type);
79-
if (
80-
manager && manager.sanitizeFormField &&
81-
!skipFields.includes(widget.fieldName)
82-
) {
83-
try {
84-
manager.checkRequired(req, widget, input);
85-
await manager.sanitizeFormField(widget, input, output);
86-
} catch (err) {
87-
if (err.data && err.data.fieldError) {
88-
formErrors.push(err.data.fieldError);
89-
} else {
90-
throw err;
94+
self.collectToSkip(input, conditionals, skipFields);
95+
96+
for (const area of areas) {
97+
const widgets = area.items || [];
98+
for (const widget of widgets) {
99+
const manager = self.apos.area.getWidgetManager(widget.type);
100+
if (
101+
manager && manager.sanitizeFormField &&
102+
!skipFields.includes(widget.fieldName)
103+
) {
104+
try {
105+
manager.checkRequired(req, widget, input);
106+
await manager.sanitizeFormField(widget, input, output);
107+
} catch (err) {
108+
if (err.data && err.data.fieldError) {
109+
formErrors.push(err.data.fieldError);
110+
} else {
111+
throw err;
112+
}
91113
}
92114
}
93115
}
94116
}
95-
}
96117

97-
if (formErrors.length > 0) {
98-
throw self.apos.error('invalid', {
99-
formErrors
100-
});
118+
if (formErrors.length > 0) {
119+
throw self.apos.error('invalid', {
120+
formErrors
121+
});
122+
}
123+
} catch (error) {
124+
// The submission was rejected after one or more uploaded files were
125+
// stored (a field validation error, a file upload failure, or an
126+
// unexpected error). Remove those attachments so a rejected submission
127+
// can't leave persistent, publicly accessible files behind.
128+
await self.deleteFieldFiles(req, insertedAttachmentIds);
129+
throw error;
101130
}
102131

103132
if (form.enableQueryParams && form.queryParamList.length > 0) {
@@ -128,6 +157,27 @@ module.exports = function (self) {
128157
return ids;
129158

130159
},
160+
async deleteFieldFiles (req, ids = []) {
161+
// Remove attachments that were stored while processing a form submission
162+
// that was ultimately rejected. Both the stored files (in uploadfs) and
163+
// the database record are removed so no publicly accessible orphan
164+
// remains. Best effort: a failure to remove one attachment must not
165+
// prevent removal of the others, nor mask the original submission error.
166+
for (const id of ids) {
167+
try {
168+
const attachment = await self.apos.attachment.db.findOne({ _id: id });
169+
170+
if (!attachment) {
171+
continue;
172+
}
173+
174+
await self.apos.attachment.alterAttachment(attachment, 'remove');
175+
await self.apos.attachment.db.removeOne({ _id: id });
176+
} catch (error) {
177+
self.apos.util.error(error);
178+
}
179+
}
180+
},
131181
matchesName(str, name) {
132182
return str.startsWith(name) && str.match(/.+-\d+$/);
133183
},

0 commit comments

Comments
 (0)