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

feat(transforms): add SES censorship evasion function #1812

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

boneskull
Copy link
Contributor

Description

Extracts the rewriteComment function from @endo/bundle-source/src/transform.js and exposes it as transformComment in the evade-censor export. This should be considered an experimental API.

Motivation

We could use this functionality in LavaMoat itself, so extracting it into a module lighter than @endo/bundle-source seems appropriate.

The function is not exported from the main entry point, nor documented publicly, as to avoid committing to a API we may not be ready to commit to. I can change this, if desired!

Notes

  • @babel/types is added as a production dependency, because the "public" API surface depends on types from this module.
  • Did not export the new function from the main entry point.
  • Added some unit tests.
  • Used homoglyphs.
  • Removed the dummy test index.
  • Added barebones README.md

Questions

  • Why do we make everything a CommentBlock?
  • Why do we replace the end-of-comment marker?
  • Is it preferable to use character escapes or no?

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

What do you think of exporting evadeCensor(source, sourceMap) => {source, sourceMap} for less public coupling?

// (featuring homoglyphs for @kriskowal)
.replace(IMPORT_RE, '\u{406}\u{16D6}\u{420}\u{39F}\u{13D2}\u{422}$2')
Copy link
Member

Choose a reason for hiding this comment

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

summoning @mhofman, resident homoglyph idea haver. (I do not need credit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason to use homoglyphs in addition to, uh, lulz?

Copy link
Member

Choose a reason for hiding this comment

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

They have the benefit of preserving the column numbers for subsequent code on the same line. But mostly lulz.

.replace(HTML_COMMENT_START_RE, '<!X-')
.replace(HTML_COMMENT_END_RE, '-X>')
Copy link
Member

Choose a reason for hiding this comment

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

Also potential homoglyph, e.g., \u2010.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Also would like:

  • unit test
  • integration test in compartment-mapper
  • refactor bundle-source in place to take on this dependency

Let’s be sure to separate the commit that changes the behavior to use homoglyphs so it can be easily reverted if necessary.

@kriskowal
Copy link
Member

I defer to @michaelfig or @erights regarding the comment end marker escape and why we replace // with /* style comments. We should capture the reasoning in the code!

I have no strong feelings about how to escape homoglphys.

Also worth noting that there are other evasive transforms we hope to add one day to provide better compatibility with the ecosystem, particularly transforms that break strings that have censored terms.

@boneskull
Copy link
Contributor Author

@kriskowal Which unit test do you mean?

@erights
Copy link
Contributor

erights commented Oct 10, 2023

why we replace // with /* style comments. We should capture the reasoning in the code!

I do not remember being aware that we changed comments this way, so I also do not remember why. If I was aware at the time, my apologies for not capturing or at least leaving more clues.

@kriskowal
Copy link
Member

@kriskowal Which unit test do you mean?

There is none as such and we are relying on integration tests inside bundle-source for the effect. I’m willing to continue in that vein.

@boneskull
Copy link
Contributor Author

There is none as such and we are relying on integration tests inside bundle-source for the effect. I’m willing to continue in that vein.

For posterity: this PR does include some unit tests.

@boneskull
Copy link
Contributor Author

What do you think of exporting evadeCensor(source, sourceMap) => {source, sourceMap} for less public coupling?

@kriskowal

This sounds like I'd need to copy makeLocationUnmapper() and other functions from bundle-source/src/transform.js here as well.

Was the intention to rehome the entirety of bundle-source/src/transform.js into the transform pkg? Apologies if that wasn't clear. And if so, you want it all in this PR (with multiple, logical commits)?

@kriskowal
Copy link
Member

What do you think of exporting evadeCensor(source, sourceMap) => {source, sourceMap} for less public coupling?

@kriskowal

This sounds like I'd need to copy makeLocationUnmapper() and other functions from bundle-source/src/transform.js here as well.

Was the intention to rehome the entirety of bundle-source/src/transform.js into the transform pkg? Apologies if that wasn't clear. And if so, you want it all in this PR (with multiple, logical commits)?

I had not thought that many steps ahead, but that would seem to be necessary in order to decouple the public interface from the encapsulated dependencies, so I’m in favor.

@boneskull boneskull force-pushed the boneskull/transform branch from fc26c2f to de80b75 Compare October 16, 2023 22:42
@boneskull
Copy link
Contributor Author

this is getting closer, but I need to add some more tests. I'm not sure how unit-y I can make them, though...

@boneskull boneskull force-pushed the boneskull/transform branch 2 times, most recently from 7f6e715 to 059e724 Compare October 17, 2023 21:00
@boneskull
Copy link
Contributor Author

@kriskowal Please let me know if my test fixtures in @endo/transforms are overkill

@boneskull
Copy link
Contributor Author

I suppose I should update README.md with API docs too

@boneskull boneskull force-pushed the boneskull/transform branch 3 times, most recently from 564fe4b to 07c843b Compare October 19, 2023 00:14
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

The substance of this change is perfect. I challenge you to consider my comments about anticipating future changes in the names and paths of functions and modules.

packages/transforms/README.md Outdated Show resolved Hide resolved
packages/transforms/README.md Outdated Show resolved Hide resolved
packages/transforms/README.md Outdated Show resolved Hide resolved
@boneskull boneskull force-pushed the boneskull/transform branch 2 times, most recently from 4e90108 to 83d0fb2 Compare October 20, 2023 22:24
@boneskull
Copy link
Contributor Author

@kriskowal OK, I've

  1. Renamed the package (I did not change the version number, however)
  2. Updated the README
  3. Narrowed the API so that only a single function is exported. Renamed it
  4. Created a couple additional adapter functions so that, ideally, swapping out the underlying AST implementation won't be so painful
  5. Accidentally discovered how to implement function overloading via JSDoc

@boneskull boneskull force-pushed the boneskull/transform branch 2 times, most recently from c44e721 to 93d3a29 Compare October 21, 2023 04:15
@boneskull
Copy link
Contributor Author

linefeeds. lol

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Left some homoglyph suggestions that should make comments look the same to a developer in a debugger without tripping the censor.

packages/evasive-transform/src/transform-comment.js Outdated Show resolved Hide resolved
packages/evasive-transform/src/transform-comment.js Outdated Show resolved Hide resolved
packages/evasive-transform/src/transform-comment.js Outdated Show resolved Hide resolved
@boneskull boneskull force-pushed the boneskull/transform branch 4 times, most recently from 943d3b9 to b81d6d2 Compare October 23, 2023 22:07
@boneskull
Copy link
Contributor Author

OK. Don't touch anything. @kriskowal can I be done please

@boneskull
Copy link
Contributor Author

8d9d2b7 was committed from windows where I am evidently not setup properly with gpg.

@kriskowal
Copy link
Member

Thanks, @boneskull. You’re green. Would you like to squash some or all of the commits or shall I command Github to do so on your behalf?

… package

This extracts everything from `@endo/bundle-source/src/transform.js` into a new package (formerly `@endo/transforms`).

LavaMoat has need to consume the SES-censorship-evasion functionality, and it would be cumbersome to try to use `@endo/bundle-source` for this.
This jettisons `src/transform.js` and consumes `@endo/evasive-transform` instead.
This normalizes linebreaks to `lf`.  Sorry, Windows contributors.

Additionally, forces `.snap` and `.agar` to be binary, if not already recognized as such.
@boneskull boneskull force-pushed the boneskull/transform branch from b81d6d2 to ce8f3f0 Compare October 25, 2023 22:28
@boneskull
Copy link
Contributor Author

@kriskowal Squashed

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.

3 participants