Skip to content
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

Implement access tracking for containingUrl #285

Merged
merged 2 commits into from Apr 17, 2024
Merged

Implement access tracking for containingUrl #285

merged 2 commits into from Apr 17, 2024

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Apr 12, 2024

@ntkme
Copy link
Contributor Author

ntkme commented Apr 12, 2024

Looks like yesterday's node 20.12.2 is breaking our test on windows: nodejs/node@69ffc6d50d

@ntkme
Copy link
Contributor Author

ntkme commented Apr 12, 2024

@nex3 I added a commit to fix the node regression.

Regarding CVE-2024-27980:

  • sass-embedded >=1.59.2 is not affected because in production release we always launch the dart.exe directly since this commit: 3088620
  • sass-embedded <1.59.2 would be broken if a windows user upgrades to the latest node. There is no security risk for sass-embedded itself even if a windows user does not upgrade node, because we don't have any arguments when launching .bat, thus there is no risk of injection.
  • Test is broken because we use the .bat wrapper in the test.

@nex3
Copy link
Contributor

nex3 commented Apr 12, 2024

Can you pull the CVE fix into a separate PR?

@ntkme
Copy link
Contributor Author

ntkme commented Apr 12, 2024

@nex3 Created a separate PR for the CVE: #286

lib/src/importer-registry.ts Outdated Show resolved Hide resolved
* it's not part of the package's public API and should not be accessed by
* user code. It may be renamed or removed without warning in the future.
*/
containingUrlAccessed = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this field private and give it a public getter, so it's clear that only this class should ever modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I also made the same change for ArugmentList, which is where I copied the pattern.

@nex3 nex3 merged commit 3080e76 into sass:main Apr 17, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants