H-6498: Increase printWidth across oxfmt to 100#8745
Open
indietyp wants to merge 2 commits into
Open
Conversation
PR SummaryLow Risk Overview No functional logic or API behavior is intentionally changed; diffs are overwhelmingly whitespace/line-break adjustments to match the new formatter output. Reviewed by Cursor Bugbot for commit 5bf3cce. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8745 +/- ##
==========================================
- Coverage 58.82% 58.73% -0.10%
==========================================
Files 1328 1328
Lines 126882 126992 +110
Branches 5790 5809 +19
==========================================
- Hits 74644 74583 -61
- Misses 51357 51528 +171
Partials 881 881 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🤖 Augment PR SummarySummary: This PR increases the configured Changes:
Technical Notes: The diff appears to be formatter-only (no functional changes intended); the main behavioral change is the formatter configuration update, which will affect pre-commit/CI formatting checks that run 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .send( | ||
| `No data sent with ${payload.action} ${payload.type} webhook payload`, | ||
| ); | ||
| res.status(400).send(`No data sent with ${payload.action} ${payload.type} webhook payload`); |
| res | ||
| .status(400) | ||
| .send(`No ID found in ${payload.action} ${payload.type} webhook payload`); | ||
| res.status(400).send(`No ID found in ${payload.action} ${payload.type} webhook payload`); |
| "state", | ||
| `"use strict"; var ${SHADOWED_GLOBALS}; ${code}`, | ||
| ) as (state: MetricState) => unknown; | ||
| rawFn = new Function("state", `"use strict"; var ${SHADOWED_GLOBALS}; ${code}`) as ( |
| const constructorPattern = new RegExp( | ||
| `export\\s+default\\s+${constructorFnName}\\s*\\(`, | ||
| ); | ||
| const constructorPattern = new RegExp(`export\\s+default\\s+${constructorFnName}\\s*\\(`); |
| @@ -6,8 +6,7 @@ | |||
| * The base URL of the User entity type, used to detect user entities | |||
| * when enforcing property update restrictions. | |||
| */ | |||
| export const userEntityTypeBaseUrl = | |||
| "https://hash.ai/@h/types/entity-type/user/" as BaseUrl; | |||
| export const userEntityTypeBaseUrl = "https://hash.ai/@h/types/entity-type/user/" as BaseUrl; | |||
![semgrep-code-hashintel[bot]](https://avatars.githubusercontent.com/u/22116112?s=60&v=4){kind=small}
| const constructorPattern = new RegExp( | ||
| `export\\s+default\\s+${constructorFnName}\\s*\\(`, | ||
| ); | ||
| const constructorPattern = new RegExp(`export\\s+default\\s+${constructorFnName}\\s*\\(`); |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
User-controlled function name is used to construct a RegExp pattern without sanitization, allowing ReDoS attacks that can freeze the application's main thread.
More details about this
The constructorPattern RegExp is built from constructorFnName, a user-provided function argument. An attacker could pass a maliciously crafted constructorFnName containing regex metacharacters or patterns that cause catastrophic backtracking when the pattern is tested against sanitizedCode.
Exploit scenario:
- An attacker calls
compileUserCode(code, "a+a+a+a+b")with a pathological regex pattern as the constructor name - When
constructorPattern.test(sanitizedCode)is called, the RegExp engine enters catastrophic backtracking trying to match the malicious pattern against even moderately-sized code strings - The main thread blocks indefinitely, causing a Denial-of-Service and preventing the application from handling other requests
For example, with constructorFnName = "(a|a)*b" and a code string without that pattern, the regex engine will try exponentially many combinations before failing, freezing the application.
Dataflow graph
flowchart LR
classDef invis fill:white, stroke: none
classDef default fill:#e7f5ff, color:#1c7fd6, stroke: none
subgraph File0["<b>libs/@hashintel/petrinaut-core/src/simulation/authoring/user-code/compile-user-code.ts</b>"]
direction LR
%% Source
subgraph Source
direction LR
v0["<a href=https://github.com/hashintel/hash/blob/5bf3cceaa4c0073a1b1a4259a02ccd867933fff5/libs/@hashintel/petrinaut-core/src/simulation/authoring/user-code/compile-user-code.ts#L44 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 44] constructorFnName</a>"]
end
%% Intermediate
subgraph Traces0[Traces]
direction TB
v2["<a href=https://github.com/hashintel/hash/blob/5bf3cceaa4c0073a1b1a4259a02ccd867933fff5/libs/@hashintel/petrinaut-core/src/simulation/authoring/user-code/compile-user-code.ts#L44 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 44] constructorFnName</a>"]
v3["<a href=https://github.com/hashintel/hash/blob/5bf3cceaa4c0073a1b1a4259a02ccd867933fff5/libs/@hashintel/petrinaut-core/src/simulation/authoring/user-code/compile-user-code.ts#L58 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 58] `</a>"]
end
v2 --> v3
%% Sink
subgraph Sink
direction LR
v1["<a href=https://github.com/hashintel/hash/blob/5bf3cceaa4c0073a1b1a4259a02ccd867933fff5/libs/@hashintel/petrinaut-core/src/simulation/authoring/user-code/compile-user-code.ts#L58 target=_blank style='text-decoration:none; color:#1c7fd6'>[Line: 58] new RegExp(`export\\s+default\\s+${constructorFnName}\\s*\\(`)</a>"]
end
end
%% Class Assignment
Source:::invis
Sink:::invis
Traces0:::invis
File0:::invis
%% Connections
Source --> Traces0
Traces0 --> Sink
To resolve this comment:
✨ Commit fix suggestion
Suggested change
| const constructorPattern = new RegExp(`export\\s+default\\s+${constructorFnName}\\s*\\(`); | |
| export function compileUserCode<T extends unknown[] = unknown[]>( | |
| code: string, | |
| constructorFnName: string, | |
| ): (...args: T) => unknown { | |
| // Validate the constructor function name before using it in generated code. | |
| // This ensures it is a safe JavaScript identifier and avoids dynamic regex usage. | |
| const safeIdentifierRegex = /^[A-Za-z_$][A-Za-z0-9_$]*$/; | |
| if (!safeIdentifierRegex.test(constructorFnName)) { | |
| throw new Error( | |
| `Invalid constructor function name '${constructorFnName}'. Expected a valid JavaScript identifier.`, | |
| ); | |
| } | |
| const validatedConstructorFnName = constructorFnName; | |
| // Strip TypeScript type annotations and remove leading/trailing whitespace | |
| const sanitizedCode = stripTypeAnnotations(code.trim()); | |
| // Verify that the code has a default export | |
| const defaultExportRegex = /export\s+default\s+/; | |
| if (!defaultExportRegex.test(sanitizedCode)) { | |
| throw new Error( | |
| `Module must have a default export. Expected pattern: export default ${validatedConstructorFnName}(...)`, | |
| ); | |
| } | |
| // Verify that the default export uses the specified constructor function | |
| const expectedExport = `export default ${validatedConstructorFnName}`; | |
| const exportIndex = sanitizedCode.indexOf(expectedExport); | |
| let hasExpectedConstructorCall = false; | |
| if (exportIndex !== -1) { | |
| let nextIndex = exportIndex + expectedExport.length; | |
| while ( | |
| nextIndex < sanitizedCode.length && | |
| /\s/.test(sanitizedCode[nextIndex]) | |
| ) { | |
| nextIndex += 1; | |
| } | |
| hasExpectedConstructorCall = sanitizedCode[nextIndex] === "("; | |
| } | |
| if (!hasExpectedConstructorCall) { | |
| throw new Error( | |
| `Default export must use the constructor function '${validatedConstructorFnName}'. Expected pattern: export default ${validatedConstructorFnName}(...)`, | |
| ); | |
| } | |
| try { | |
| // Create a mock constructor function that extracts the wrapped function | |
| const mockConstructor = ` | |
| function ${validatedConstructorFnName}(fn) { | |
| if (typeof fn !== 'function') { | |
| throw new Error('${validatedConstructorFnName} expects a function as argument'); | |
| } | |
| return fn; | |
| } | |
| `; | |
| // Create an executable module-like environment | |
| const executableCode = ` | |
| ${distributionRuntimeCode} | |
| ${mockConstructor} | |
| let __default_export__; | |
| ${sanitizedCode.replace(/export\s+default\s+/, "__default_export__ = ")} | |
| return __default_export__; | |
| `; | |
| // Use Function constructor to create and execute the module | |
| // eslint-disable-next-line no-new-func, @typescript-eslint/no-implied-eval, @typescript-eslint/no-unsafe-call | |
| const compiledFunction = new Function(executableCode)() as ( | |
| ...args: T |
View step-by-step instructions
- Remove the dynamic
RegExpconstructor that usesconstructorFnName. - Build the expected export prefix as a plain string, for example
const expectedExport = \export default ${constructorFnName}`;`. - Check for the constructor call with string operations instead of regex, such as finding
expectedExportinsanitizedCodeand then verifying the next non-space character after the constructor name is(. - Keep the existing static regex for the generic
export defaultcheck, because/export\s+default\s+/is hardcoded and does not use tainted input. - Validate
constructorFnNamebefore using it elsewhere in generated code so it only contains a safe JavaScript identifier, for example by checking it against a hardcoded regex like/^[A-Za-z_$][A-Za-z0-9_$]*$/. This prevents malformed names from breaking the parser and avoids needing dynamic regex escaping. - Update the error path to use the same validated
constructorFnNamevalue when reporting the expected pattern.
Alternatively, if you need to preserve flexible whitespace matching, split the logic into simple string parsing steps instead of new RegExp(...), for example by locating export default, trimming the following text, and comparing the next token to constructorFnName.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by detect-non-literal-regexp.
You can view more details about this finding in the Semgrep AppSec Platform.
Contributor
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1001 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1526 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 51 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 107 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 25 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |
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
2 participants
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.
🌟 What is the purpose of this PR?
Increase the printWidth to 100, touches a lot of files because it needs to.
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR: