Fix the polynomial-ReDoS CodeQL alert on toAbsoluteMediaUrl - #2009
Merged
Conversation
`/\/+$/` is quadratic when the run of slashes does not end the string — 2.8s for 100k on this machine — and `shared` is a published library, so the argument is the caller's (CodeQL js/polynomial-redos, alert 51). Adds the spec the helper never had: it moved into shared from two copies that had none, covered only indirectly through the app's resolveVideoSource.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes CodeQL alert 51 (
js/polynomial-redos, security-severity high) onshared/src/util/mediaUrl.ts:26, raised against #2007.Is it real?
The regex is real, the exposure is mild.
/\/+$/backtracks quadratically when a run of slashes does not end the string: the engine restarts at each slash, matches the rest of the run, then fails on$. Measured on this machine:So the shape that bites is
"/////…x", not a URL with a trailing slash.What CodeQL means by "library input" is the exported parameter itself:
sharedis published asluminary-shared, sopublicUrlis whatever a caller passes. In our callers it is a Storage document'spublicUrl, set by someone holding Assign on the bucket — an admin who could break media far more directly. That is why this is worth fixing but not worth alarm.Fixing beats dismissing here: the replacement is shorter than the regex, and it removes the argument rather than filing it.
The fix
Trailing slashes come off by index, in one linear scan. Same result for every input, including the several-trailing-slashes case the regex handled.
Tests
shared/src/util/mediaUrl.tshad no spec — it was moved intosharedfrom two copies that had none either, and onlyapp'sresolveVideoSourcecovered it indirectly. Added one: the join, external URLs passing through, the undefined cases, one and many trailing slashes, an all-slashes bucket URL, and the pathological input under a time bound.Confirmed the timing test earns its place: with the old regex restored it fails, with the fix it passes. (My first attempt at that fixture put the slash run at the end of the string, where the old regex is instant — it would have passed against the bug. Corrected to the shape that actually backtracks.)
Left alone
The same
replace(/\/+$/, "")appears eight more times —api/src/changeRequests/documentProcessing/mediaUrl.ts(including the direct twin of this function),deleteMediaCollection.ts,migrateMediaCollection.ts,v17.ts,util/authority.ts, andcms/.../AuthConfig.vue. None is a library entry point, none is flagged, and all predate this epic. Happy to sweep them in a follow-up if you'd rather they matched.