-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(conditional-page-builds): track potentially unsafe Node.js builtin modules usage #29560
Changes from all commits
cea1d9f
dd9ce25
84b9c57
d495cd8
197cb62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
body { | ||
background: yellow; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
import PostBodyComponents from "./src/components/post-body-components-ssr" | ||
import * as React from "react" | ||
import * as fs from "fs" | ||
|
||
export function onRenderBody({ setPostBodyComponents }) { | ||
setPostBodyComponents(PostBodyComponents) | ||
export function onRenderBody({ setHeadComponents }) { | ||
setHeadComponents( | ||
<style | ||
dangerouslySetInnerHTML={{ | ||
__html: `body {\nbackground: white;\n}`, | ||
}} | ||
/> | ||
) | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
/* eslint-disable filenames/match-regex */ | ||
const { wrapModuleWithTracking } = require(`./tracking-unsafe-module-wrapper`) | ||
|
||
module.exports = wrapModuleWithTracking(`child_process`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const { wrapModuleWithTracking } = require(`./tracking-unsafe-module-wrapper`) | ||
|
||
module.exports = wrapModuleWithTracking(`fs`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const { wrapModuleWithTracking } = require(`./tracking-unsafe-module-wrapper`) | ||
|
||
module.exports = wrapModuleWithTracking(`http`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const { wrapModuleWithTracking } = require(`./tracking-unsafe-module-wrapper`) | ||
|
||
module.exports = wrapModuleWithTracking(`http2`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const { wrapModuleWithTracking } = require(`./tracking-unsafe-module-wrapper`) | ||
|
||
module.exports = wrapModuleWithTracking(`https`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
// initializing global here for unsafe builtin usage at import time | ||
global.unsafeBuiltinUsage = [] | ||
|
||
function createProxyHandler(prefix) { | ||
return { | ||
get: function (target, key) { | ||
const value = target[key] | ||
if (typeof value === `function`) { | ||
return function wrapper(...args) { | ||
const myErrorHolder = { | ||
name: `Unsafe builtin usage ${prefix}.${key}`, | ||
} | ||
Error.captureStackTrace(myErrorHolder, wrapper) | ||
|
||
global.unsafeBuiltinUsage.push(myErrorHolder.stack) | ||
return value.apply(target, args) | ||
} | ||
} else if (typeof value === `object` && value !== null) { | ||
return new Proxy( | ||
value, | ||
createProxyHandler( | ||
key && key.toString ? `${prefix}.${key.toString()}` : prefix | ||
) | ||
) | ||
} | ||
|
||
return value | ||
}, | ||
} | ||
} | ||
|
||
function wrapModuleWithTracking(moduleName) { | ||
const mod = require(moduleName) | ||
return new Proxy(mod, createProxyHandler(moduleName)) | ||
} | ||
|
||
exports.wrapModuleWithTracking = wrapModuleWithTracking |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,6 +103,10 @@ export default ({ | |
reversedStyles, | ||
reversedScripts, | ||
}) => { | ||
// for this to work we need this function to be sync or at least ensure there is single execution of it at a time | ||
global.unsafeBuiltinUsage = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hurts my feelings, but I essentially need "context" that "unconnected" arbitrary code can access and push to. |
||
|
||
try { | ||
let bodyHtml = `` | ||
let headComponents = [ | ||
<meta | ||
|
@@ -134,7 +138,9 @@ export default ({ | |
} | ||
|
||
const setPreBodyComponents = components => { | ||
preBodyComponents = preBodyComponents.concat(sanitizeComponents(components)) | ||
preBodyComponents = preBodyComponents.concat( | ||
sanitizeComponents(components) | ||
) | ||
} | ||
|
||
const setPostBodyComponents = components => { | ||
|
@@ -413,5 +419,9 @@ export default ({ | |
/> | ||
)}` | ||
|
||
return html | ||
return { html, unsafeBuiltinsUsage: global.unsafeBuiltinUsage } | ||
} catch (e) { | ||
e.unsafeBuiltinsUsage = global.unsafeBuiltinUsage | ||
throw e | ||
Comment on lines
+422
to
+425
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did anything change except this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest checking with "hide whitespace changes" ( https://github.com/gatsbyjs/gatsby/pull/29560/files?w=1#diff-4cd5b620539129dfa79254b9cdae3bd9b619426b83b412d2647cfb416730d362 ) This whole file added
at the start of the function, changed return to return object instead of string and wrapped everything in try/catch to decorate caught error with |
||
} | ||
} |
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.
this is because filename (
child_process.js
doesn't match our eslint rule :( )