-
Notifications
You must be signed in to change notification settings - Fork 983
security(star-rating): authorize the /o?method=star ratings read (24.05) #7956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ar2rsawseen
wants to merge
3
commits into
release.24.05
Choose a base branch
from
backport/star-method-authorize-2405
base: release.24.05
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+215
−62
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| require("should"); | ||
| var fs = require("fs"); | ||
| var path = require("path"); | ||
|
|
||
| // /o/feedback/multiple/versions groups stored ratings by platform, using the platform | ||
| // name as an object key. That name arrives on the PUBLIC star-rating event, in the | ||
| // platform_version_rate segmentation, so anyone who can write to the app decides it. | ||
| // | ||
| // On a plain object, reading back a key such as "__proto__", "constructor" or "toString" | ||
| // returns an inherited member rather than undefined, so the "not seen yet" branch never | ||
| // runs, the array is never created, and the .indexOf() on the next line throws. The read | ||
| // then fails for every authorized caller until the seeded rows age out - the endpoint is | ||
| // denied by data someone else planted. | ||
| // | ||
| // The accumulation is an inline callback, so it is lifted out of the real source and run | ||
| // here. Lifting keeps the proof on the shipping code: dropping Object.create(null) breaks | ||
| // a behavioural test, not only a source match. | ||
|
|
||
| var API = path.join(__dirname, "../../plugins/star-rating/api/api.js"); | ||
| var src = fs.readFileSync(API, "utf8"); | ||
| var lines = src.split("\n"); | ||
|
|
||
| /** | ||
| * Lift the statement starting at the first line ending with `head`, through the first | ||
| * following line whose text is `close` at the same indentation | ||
| * @param {string} head - how the statement's first line ends | ||
| * @param {string} close - the closing text, without indentation | ||
| * @returns {string} the lifted statement, left trimmed | ||
| */ | ||
| function lift(head, close) { | ||
| var start = lines.findIndex(function(l) { | ||
| return l.trimEnd().endsWith(head); | ||
| }); | ||
| if (start < 0) { | ||
| throw new Error("not found in star-rating api.js: " + head); | ||
| } | ||
| var indent = lines[start].match(/^\s*/)[0]; | ||
| for (var j = start + 1; j < lines.length; j++) { | ||
| if (lines[j] === indent + close) { | ||
| return lines.slice(start, j + 1).map(function(l) { | ||
| return l.slice(indent.length); | ||
| }).join("\n"); | ||
| } | ||
| } | ||
| throw new Error("close not found for: " + head); | ||
| } | ||
|
|
||
| var LIFTED = lift("doc.meta.platform_version_rate.forEach(function(item) {", "});"); | ||
|
|
||
| // the accumulator's declaration is lifted too, not written here: which object it is IS | ||
| // the fix, so a test that built its own would pass either way | ||
| var LIFTED_INIT = (function() { | ||
| var at = lines.findIndex(function(l) { | ||
| return l.trimEnd().endsWith("doc.meta.platform_version_rate.forEach(function(item) {"); | ||
| }); | ||
| for (var j = at; j >= 0; j--) { | ||
| if (/^\s*var result = .+;\s*$/.test(lines[j])) { | ||
| return lines[j].trim(); | ||
| } | ||
| } | ||
| throw new Error("accumulator declaration not found in star-rating api.js"); | ||
| }()); | ||
|
|
||
| /** | ||
| * Run the real accumulation over the given rating segmentation values | ||
| * @param {Array} values - platform_version_rate entries as stored | ||
| * @returns {object} the grouping the endpoint would return | ||
| */ | ||
| function accumulate(values) { | ||
| var result; | ||
| var doc = {meta: {platform_version_rate: values}}; | ||
| /* eslint-disable no-eval */ | ||
| eval(LIFTED_INIT); | ||
| eval(LIFTED); | ||
| /* eslint-enable no-eval */ | ||
| return result; | ||
| } | ||
|
|
||
| describe("star-rating platform grouping", function() { | ||
| it("builds its accumulator with a null prototype", function() { | ||
| // asserted on the source as well, because the whole defect is which object the | ||
| // accumulator is: an edit back to {} would be invisible in a diff review | ||
| var accumulators = lines.filter(function(l) { | ||
| return l.indexOf("var result = Object.create(null);") > -1; | ||
| }); | ||
| accumulators.length.should.be.above(0); | ||
| lines.filter(function(l, at) { | ||
| return /^\s*var result = \{\};\s*$/.test(l) | ||
| && lines.slice(at, at + 45).join("\n").indexOf("result[data[0]]") > -1; | ||
| }).should.eql([]); | ||
| }); | ||
|
|
||
| it("groups ordinary platforms by name", function() { | ||
| var out = accumulate([ | ||
| "Android**1.0**5**w1**", | ||
| "Android**1.1**4**w1**", | ||
| "iOS**2.0**5**w1**", | ||
| "Android**1.0**3**w1**" | ||
| ]); | ||
| Object.keys(out).sort().should.eql(["Android", "iOS"]); | ||
| out.Android.should.eql(["1.0", "1.1"]); | ||
| out.iOS.should.eql(["2.0"]); | ||
| }); | ||
|
|
||
| it("survives a platform named after a prototype member", function() { | ||
| // each of these read back as an inherited member on a plain object: an object, | ||
| // the Object function, and a function respectively. None is undefined, so the | ||
| // array was never created and .indexOf threw a TypeError on the next line. | ||
| ["__proto__", "constructor", "toString", "valueOf", "hasOwnProperty"].forEach(function(name) { | ||
| var out = null; | ||
| var thrown = null; | ||
| try { | ||
| out = accumulate([name + "**1.0**5**w1**"]); | ||
| } | ||
| catch (e) { | ||
| thrown = e; | ||
| } | ||
| (thrown === null).should.equal(true, name + " threw: " + (thrown && thrown.message)); | ||
| out[name].should.eql(["1.0"]); | ||
| }); | ||
| }); | ||
|
|
||
| it("still answers about the other platforms when one is seeded", function() { | ||
| // the point of the fix: one planted row must not deny the whole read | ||
| var out = accumulate([ | ||
| "Android**1.0**5**w1**", | ||
| "__proto__**9.9**1**w1**", | ||
| "iOS**2.0**5**w1**" | ||
| ]); | ||
| out.Android.should.eql(["1.0"]); | ||
| out.iOS.should.eql(["2.0"]); | ||
| }); | ||
|
|
||
| it("serialises to the response the caller expects", function() { | ||
| // returnOutput stringifies it, and a null prototype object stringifies the same | ||
| var out = accumulate(["__proto__**1.0**5**w1**", "Android**1.0**5**w1**"]); | ||
| JSON.parse(JSON.stringify(out)).should.have.property("Android"); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Use a null-prototype map for stored platform names
data[0]comes from the public star-rating event'splatform_version_ratesegmentation and is used as an object key. Values such as__proto__,constructor, ortoStringresolve inherited members on{}, so this condition does not initialize an array and the following.indexOf()throws, making the authorized ratings read fail on attacker-seeded data. BuildresultwithObject.create(null)or use an own-property check and explicitly initialize each key. The equivalent accumulator in the alternate/granular branch needs the same treatment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, and it is a denial rather than a corruption — fixed in 2e71724.
Ran the loop rather than reasoning about it. Against a plain object, each of these throws:
for
__proto__(reads back asObject.prototype),constructor(theObjectfunction), andtoString/valueOf/hasOwnProperty(functions). None isundefined, so the "not seen yet" branch never runs and the array is never created. One planted row therefore fails the read for every authorized caller until it ages out — and the name comes off the public star-rating event'splatform_version_ratesegmentation, so anyone who can write to the app chooses it.Took the
Object.create(null)option:Nothing downstream changes — the keys are still ordinary strings, and
JSON.stringifyserialises a null-prototype object identically, which is whatreturnOutputdoes with it. There is a test pinning that.And the granular branch too, as you asked: the platform copy has a second accumulator over
data2.data[z]._id.split('**')and it gets the same treatment. The server copies only have the one.On the tests: they lift both the accumulator's declaration and the loop out of the real source, rather than the loop alone. That matters here — a test that built its own
resultwould pass whichever object the shipping code used. Four of the five fail against the previous code, with the TypeError above.