feat(playground): support safe-mode script libraries in the quickjs sandbox - #56
feat(playground): support safe-mode script libraries in the quickjs sandbox#56sundram-bruno wants to merge 18 commits into
Conversation
32064b1 to
7ddd5c0
Compare
- require absolute http(s) urls and omit credentials in the axios shim - reject asymmetric keys and missing secrets in the jwt helper - keep the crypto shim working under minified builds - replace local-file require with a clear error and remove the unused loader - strip __proto__ when marshalling host values into the sandbox
- add jwt guard, marshalling and node: builtin parity cases - rewrite the script-execution e2e to a self-contained tests script - drop the now-unused sample collection script
…ipt-libraries # Conflicts: # package-lock.json # packages/bruno-api-docs/package.json
… callbacks - omit typ and reject claim options for string payloads - invoke the callback at most once - add node crypto interop tests for HS signatures
…ipt-libraries # Conflicts: # packages/bruno-api-docs/src/components/Playground/Content/Views/PlaygroundView/PlaygroundView.tsx
jwt signing and verifying are backend operations that need a secret,
which should not live in published docs. require('jsonwebtoken') now
returns a clear not-supported error.
- guard require against inherited prototype keys and point crypto at the global - drop unsupported BigInt typed arrays and reject them with a clear message - throw on unmarshallable values instead of returning undefined - import the node build of atob directly and add its type declaration - assorted test and hook-dependency nits
vasharma05-bruno
left a comment
There was a problem hiding this comment.
Some final changes, else looks good
| // Never send the docs site's own cookies, so a published script cannot make | ||
| // authenticated calls to the host origin on the reader's behalf. |
There was a problem hiding this comment.
Do we need this comment?
| ); | ||
| } | ||
| if (${JSON.stringify(UNSUPPORTED_LIBRARIES)}.includes(bareName)) { | ||
| throw new Error(\`'\${mod}' is not currently supported in the docs playground.\`); |
There was a problem hiding this comment.
Should we use "Please use the Bruno desktop app" as err message? Similar to our other unsupported messages
There was a problem hiding this comment.
Whats the reason for this change? do we need it for this PR or is it a separate thing? As it changes auth priority for every req now and now script's set Authorization header wins over the auth configs.
for example: a request has no auth of its own so it inherits the collection's auth like "Bearer token". If a pre-req script also sets an Authorization header the script's header now takes priority.
Before the collection's auth would have been used instead.
There was a problem hiding this comment.
@sundram-bruno Can we move it to its own PR so the auth change gets reviewed properly on its own?
| import { Buffer } from 'buffer'; | ||
| import moment from 'moment'; | ||
| import btoa from 'btoa'; | ||
| import atob from 'atob/node-atob'; |
There was a problem hiding this comment.
atob is imported from atob/node-atob here and the normal import resolves to a browser shim that wont run in quickjs. Can we add a one line comment here so nobody "fixes" it later and breaks it.
| }); | ||
|
|
||
| it('exposes the supported safe-mode globals', () => { | ||
| const globals = ['expect', 'assert', 'moment', 'btoa', 'atob', 'Buffer', 'tv4', 'Ajv', 'addFormats', 'crypto', 'axios', 'path', 'require']; |
There was a problem hiding this comment.
This globals list skips uuid and nanoid
| } | ||
| `; | ||
| }; | ||
| addCryptoUtilsShimToContext(vm); |
There was a problem hiding this comment.
crypto shim installs before the bundle and then uuid and nanoid is uses it.
Lets add a one line comment so nobody reorders it by accident.
| if (response.status < 200 || response.status >= 300) { | ||
| throw buildAxiosError( | ||
| { | ||
| message: `Request failed with status code ${response.status}`, | ||
| code: response.status >= 500 ? 'ERR_BAD_RESPONSE' : 'ERR_BAD_REQUEST', | ||
| response: { | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| headers: responseHeaders, | ||
| data | ||
| } | ||
| }, | ||
| config | ||
| ); | ||
| } |
There was a problem hiding this comment.
validateStatus is being ignored here as we always throw on a non 2xx status. On the app side it works, so a script that uses validateStatus to read a 404 without a try catch breaks here .
Instead of silently dropping options we dont support, can we throw a clear error like we do for the unsupported methods below?
| } | ||
|
|
||
| let body: string | undefined; | ||
| if (config.data !== undefined && config.data !== null) { |
There was a problem hiding this comment.
it builds a body for any method even for GET
There was a problem hiding this comment.
if you accidentally pass a body to a GET request axios.get(url, { data: {...} }) axios just ignores the body and the req works fine.
Currently what we are doing is building the body for GET and handing it to the browser's fetch, which rejects it. The shim catches that rejection and reports it as a generic "Network Error" with a CORS.
So the user sees "Network Error, maybe it's CORS" when the real problem is simply "body on a GET."
| const query = new URLSearchParams(); | ||
| Object.entries(config.params).forEach(([key, value]) => { | ||
| if (value !== undefined && value !== null) { | ||
| query.append(key, String(value)); |
There was a problem hiding this comment.
If a script passes an array as a query param like params: { ids: [1, 2] } in real axios it turns it into ids[]=1&ids[]=2 which is the the standard repeated-key format that APIs expect.
Our shim just does String(value) on the array which produces ids=1,2. So a script sending array params builds a different URL compared to app. An API expecting ids[]=1&ids[]=2 receives ids=1,2 instead.
| const getRandomValuesHandle = vm.newFunction('getRandomValues', (arrayHandle: any) => { | ||
| try { | ||
| const serializedArray = vm.dump(arrayHandle); | ||
| const typedArray = deserializeTypedArray(serializedArray); |
There was a problem hiding this comment.
deserializeTypedArray this builds the array before the cap check on line 48
if (typedArray.length > MAX_RANDOM_BYTES_PER_CALL) {
throw new Error('getRandomValues: ArrayBufferView byte length exceeds 65536');
}
The raw __bruno__crypto__getRandomValues is exposed on the global so a script can call it directly and skip the wrapper.
There was a problem hiding this comment.
also a small obj just faking a huge length makes the host allocate much memory before we reject it which is enough to freeze the tab.
For reproducing: add a pre-req script and check the browser console
console.log('raw global:', typeof globalThis.__bruno__crypto__getRandomValues);
let msg = 'empty';
try {
globalThis.__bruno__crypto__getRandomValues({ type: 'Uint8Array', array: { length: 200000 }, length: 1 });
} catch (e) {
msg = e.message;
}
console.log('msg:', msg);
| 'Float32Array', | ||
| 'Float64Array' |
There was a problem hiding this comment.
Float32Array and Float64Array are in the supported list but web crypto only accepts integer arrays
There was a problem hiding this comment.
Can we add some unit tests that run a mocked fetch through the shim?
Jira: BRU-3838
Description
The docs playground runs collection scripts in a QuickJS sandbox, but the inbuilt script libraries available in the Bruno app were not supported there. Only chai was available, so any script using moment, crypto-js, axios or the other libraries worked in the app but failed in the playground.
Solution
The playground sandbox now supports the same safe mode library set as the Bruno app: axios, moment, crypto-js, uuid, nanoid, chai, tv4, ajv, ajv-formats, buffer, path, btoa and atob. Scripts can use them through require() or the matching globals, exactly like in the app.
Libraries that are only available in the app's developer mode now fail with a clear message instead of a generic one. Pre-request scripts that set their own Authorization header now win over the request's auth config, matching the app. The scripting engine and libraries are lazy loaded, so docs readers who never use the playground do not download any of it.
Bundle size
The scripting engine and the library bundle are loaded only on the first Send in the playground, so pages that only read docs download none of it.
First Send is a one time cost of roughly 398 KB gzipped, then served from cache. The single file standalone build cannot code split, so it inlines this instead.