refactor(ntp-si): read asset list explicitly #1046
Conversation
The ntp-si packaging logic had intimate knowledge of the internal format of `photo.json` in order to determine the set of assets to be included in the crx. This PR shifts that responsibility to `ntp-si-assets`, which as of brave/ntp-si-assets#1240 now publishes an additional file `assets.json` explicitly containing the list of files to be packaged. [example](https://mobile-data.s3.brave.com/GB/desktop/assets.json) This is a pre-req for rich NTTs, which will have a wider variety of assets. That isn't implemented server-side yet: this change is in preparation for reducing the number of places that are impacted when it is. So currently there should be no visible difference with this change: it's a pure refactor. Re #1039
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const allDownloads = json.assets.map(async asset => { | ||
| const targetAssetFilePath = path.join(targetResourceDir, asset.path) | ||
| const targetAssetFileUrl = new URL(asset.path, jsonFileUrl).href |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Are you using the URL(url, base) constructor as a security control to limit the origin with base jsonFileUrl? The base is ignored whenever url looks like an absolute URL, e.g. when it begins protocol://. \\\\ or //x.y. Verify that the URL's origin is as expected rather than relying on the URL constructor.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/url-constructor-base.yaml
Cc @thypon @kdenhartog
| const json = await response.json() | ||
|
|
||
| const allDownloads = json.assets.map(async asset => { | ||
| const targetAssetFilePath = path.join(targetResourceDir, asset.path) |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] Detected possible user input going into a path.join or path.resolve function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.
Source: https://semgrep.dev/r/javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal
Cc @thypon @kdenhartog
The ntp-si packaging logic had intimate knowledge of the internal format of
photo.jsonin order to determine the set of assets to be included in the crx.This PR shifts that responsibility to
ntp-si-assets, which as of https://github.com/brave/ntp-si-assets/pull/1240 now publishes an additional fileassets.jsonexplicitly containing the list of files to be packaged. See example.This is a pre-req for rich NTTs, which will have a wider variety of assets. That isn't implemented server-side yet: this change is in preparation for reducing the number of places that are impacted when it is. So currently there should be no visible difference with this change: it's a pure refactor.
Re #1039