Repository navigation
fix: make the suggest/permissions/datasafety/list guards actually run - #3
Open
arena-ai-coding-agent[bot] wants to merge 2 commits into
Open
arena-ai-coding-agent[bot] wants to merge 2 commits into
arena-ai-coding-agent[bot] wants to merge 2 commits into
Conversation
Three methods guard their required option with `&&` instead of `||`:
if (!opts && !opts.appId) { throw Error('appId missing') }
`!opts && !opts.appId` is only true when there is no options object at all,
and then reading `opts.appId` throws first. So the documented error can never
escape, and callers get an internals-leaking TypeError instead:
gplay.permissions() // TypeError: Cannot read properties of undefined (reading 'appId')
gplay.datasafety() // TypeError: Cannot read properties of undefined (reading 'appId')
gplay.suggest() // TypeError: Cannot read properties of undefined (reading 'term')
gplay.list() // TypeError: Cannot read properties of undefined (reading 'category')
The empty-object cases are just as bad in the other direction: `gplay.suggest({})`
skips the guard entirely and fires a batchexecute request for the literal term
`undefined`, which comes back as a "no results" answer instead of a usage error.
`app()`, `search()`, `reviews()` and `similar()` already use the `!opts ||`
form; this lines the other four up with them.
For `list()` every option is optional (README: "collection (optional, defaults
to collection.TOP_FREE)", "category (optional...)", ...), but `validate(opts)`
ran on the raw argument, so the documented `gplay.list()` call crashed while
`gplay.list({})` worked. Validation now runs on the object that already holds
the defaults, which also stops `list()` from writing `category`/`collection`
into the caller's options object on the way through.
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
`npm audit` runs as a required step of the CI workflow and is red on main (6 findings, all of them in transitive dev dependencies pulled in by mocha and eslint). None of them is reachable from the published runtime code, and none is fixable with a plain devDependency bump: even mocha@11 pins vulnerable serialize-javascript/diff ranges. Pin the affected packages through npm `overrides` and refresh the lockfile so `npm ci && npm run lint && npm test && npm audit` passes on the CI matrix (node 16/18/20). Runtime dependencies are untouched. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
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
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.
Problem
Four methods guard their required option with
&&where||is meant:!opts && !opts.appIdcan only be true when there is no options object — and in that case readingopts.appIdblows up first. So the documented error can never escape and callers get an internals-leakingTypeErrorinstead. Reproducible onmainwith no network access needed (the guard runs before the request):maingplay.suggest()TypeError: Cannot read properties of undefined (reading 'term')Error: term missinggplay.permissions()TypeError: ... (reading 'appId')Error: appId missinggplay.datasafety()TypeError: ... (reading 'appId')Error: appId missinggplay.list()TypeError: ... (reading 'category')TOP_FREE/APPLICATIONlistgplay.suggest({})batchexecuterequest for the literal termundefinedError: term missinglist()is the worse one of the five: every option is documented as optional ("collection(optional, defaults tocollection.TOP_FREE)", "category(optional, defaults to no category)"), yet the README-documentedgplay.list()throws, becausevalidate(opts)runs on the raw argument before the defaults are merged in. As a side effectvalidate()also wrotecategory/collectioninto the caller's options object.app(),search(),reviews()andsimilar()already use the!opts ||form — this lines the remaining ones up with them.Fix
lib/suggest.js,lib/permissions.js,lib/datasafety.js:!opts && !opts.x→!opts || !opts.x, so the guard covers both "no object" and "empty object".lib/list.js: merge the defaults intofullListOptsfirst and validate that, then use it for the request options, the throttle andparseCollectionAppsas well (the caller's object is no longer mutated, andfullDetaillookups inherit the samelang/country/cachedefaults as the list request itself).Errors keep being raised inside the promise executor, so they stay rejections (same as the existing
Invalid category/Invalid sortassertions in the suite) and no method's resolved value changes.Testing
New regression tests in
test/lib.suggest.js,test/lib.permissions.js,test/lib.datasafety.jsandtest/lib.list.js. The seven validation ones all fail onmainand pass here; they need no network, so they are not subject to Play flakiness:npm run lintclean;npm testotherwise unchanged.developer()has the sameif (!opts.devId)pattern, but fix: don't crash when a developer page has no app list section facundoolano/google-play-scraper#759 is already touching that file — happy to add it here if that one is not merged, or the maintainer can pick it up there.chore(deps)lockfile/overrides refresh, not part of this fix:npm audit(a required CI step) is red onmainfor every PR regardless of content, so it is included to keep CI green. Drop it / rebase once fix: resolve all npm audit findings via dependency overrides (CI audit step red) facundoolano/google-play-scraper#762 lands.