fix: add column, line, and method check to integrity check #25094

ryanthemanuel · 2022-12-10T14:03:24Z

User facing changelog

Integrity check will now fail if the column and line numbers are altered in the binary entry point.

Additional details

Previously, we would only check for column and line numbers in the entry point, but we can increase the strength of the integrity check by also checking for the specific column and line numbers of the executing code.

Steps to test

Ensure the tests pass on CI (especially the binary tests)

How has the user experience changed?

n/a

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-12-10T14:03:27Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-12-10T14:25:25Z

Test summary

199 • 0 • 26 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`8cebd44`
Started	Dec 12, 2022 8:29 PM
Ended	Dec 12, 2022 8:38 PM
Duration	09:25 💡
OS	Linux Debian -
Browser	Chrome 106

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

lmiller1990 · 2022-12-11T22:01:44Z

scripts/binary/binary-integrity-check-source.js

@@ -2,6 +2,8 @@ const OrigError = Error
 const captureStackTrace = Error.captureStackTrace
 const toString = Function.prototype.toString
 const callFn = Function.call
+const filter = Array.prototype.filter
+const startsWith = String.prototype.startsWith


Do you know why we create all these aliases in the first place instead of just doing tempError.filter(...) etc? It looks like something to do with the integrity check - how does comparing if (filter.call !== callFn) { verify the integrity?

We are protecting against the prototype call method on each of those functions being tampered with. So we want to make sure that they are still equal to the original function call prior to using them

lmiller1990 · 2022-12-11T22:03:15Z

patches/bytenode+1.3.7.dev.patch

+new file mode 100644
+index 0000000..314eeee
+--- /dev/null
+++ b/node_modules/bytenode/lib/compileFile.js


This looks hard to maintain - is there a bug in the library or we just needed a specific feature?

It is possible to write our own compileElectronCode, wrapping the bytenode library functionality, and put it in the monorepo (eg under scripts)? That seems like it'd be easier to maintain, long term, than patching the package.

The goal here is minimize the code that is in our entry point bundle in prod to prevent as much “dead code” that could be injected into as possible. Thus, I patched bytenode to split out the compilation (package time) from the loading (run time). It is definitely a little involved but I don’t think we are going to be updating that dependency very often.

emilyrohrbough · 2022-12-12T14:09:19Z

scripts/binary/binary-integrity-check-source.js

        // eslint-disable-next-line no-undef
        fileName: [appPath, 'index.js'].join(PATH_SEP),
+        line: 1,
+        column: 2076,


what are the scenarios where this column number would change? new imports into the root file?

This would change if our entry point file bundle changed at all. I don’t see that happening very frequently though and if it does, we will just update these numbers. The situations where we would update the entry point bundle is if this file changes: https://github.com/cypress-io/cypress/blob/develop/scripts/binary/binary-entry-point-source.js or if bytenode gets updated.

will this error provide insights into that column number or do we just fail and we'd have to manually verify?

What sort of insights would you expect? The error will let us know what that there was a difference and what the difference is (similar to what we were doing with file and function names previously).

sorry - i didn't remember what we were prev doing. I was just curious if this error outputs on any download or just during the build process

This error will output when the binary is corrupted. So if a user tries to open Cypress with a bad binary, they will see "your code executed from column x instead of y"

emilyrohrbough · 2022-12-12T14:19:41Z

patches/bytenode+1.3.7.dev.patch

+ * @param   {string}        [output] The output filename. (Deprecated: use args.output instead)
+ * @returns {Promise<string>}        A Promise which returns the compiled filename
+ */
+ const compileFile = async function (args, output) {


Looks like compileFile & compileElectronCode are 1:1 with the "removed" code in this patch. Can we re-generate to only remove the extra methods? Also why does it matter if we remove the other methods? Any other impact beyond making it a scoped import vs a global?

I’ll regenerate this to make sure it’s cleaner. The main reason I want to remove as much as possible is to prevent “dead code” from being in the bundle that could be deleted and new code added in its place.

I didn't realize that the binary-cleanup script was being bundled and shipped with the binary. Thought it was only used for building the binary content.

It's used for bundling but then reading the content that was bundled later as well.

Just so I understand, could you share where? Also do we need to maintain this require if it's only exporting the compileCode fn? https://github.com/cypress-io/cypress/blob/emily/delayed-ci/scripts/binary/binary-entry-point-source.js#L6

The require actually includes this code as well, but as I'm thinking through this, since I'm patching anyway I can make this a little clearer: https://github.com/bytenode/bytenode/blob/master/lib/index.js#L252

ryanthemanuel added 6 commits December 9, 2022 20:32

fix: add column, line, and method check to integrity check

c534ac4

run ci

ca01c3e

add test

6efb3ae

run ci

1c59a4c

run ci

ab85363

fix test

b71ec81

ryanthemanuel added 2 commits December 10, 2022 16:56

harden logic

4a9bcc0

trim down what is used from bytenode

8aafe38

lmiller1990 reviewed Dec 11, 2022

View reviewed changes

chrisbreiding approved these changes Dec 12, 2022

View reviewed changes

emilyrohrbough reviewed Dec 12, 2022

View reviewed changes

mjhenkes approved these changes Dec 12, 2022

View reviewed changes

ryanthemanuel added 3 commits December 12, 2022 11:01

pr comments

fe28249

fix smoke test

ea697f6

Merge branch 'develop' into ryanm/fix/column-line

54a630a

emilyrohrbough approved these changes Dec 12, 2022

View reviewed changes

ryanthemanuel added 4 commits December 12, 2022 11:45

fix build

082b495

fix build

a833145

Update cache-version.txt

a29c08c

Merge branch 'develop' into ryanm/fix/column-line

8cebd44

ryanthemanuel merged commit 8888cd9 into develop Dec 12, 2022

ryanthemanuel deleted the ryanm/fix/column-line branch December 12, 2022 20:42

flotwig pushed a commit that referenced this pull request Dec 13, 2022

fix: add column, line, and method check to integrity check (#25094)

2113392

ryanthemanuel added a commit that referenced this pull request Dec 15, 2022

fix: add column, line, and method check to integrity check (#25094)

6b94d1e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add column, line, and method check to integrity check #25094

fix: add column, line, and method check to integrity check #25094

ryanthemanuel commented Dec 10, 2022

cypress-bot bot commented Dec 10, 2022

cypress bot commented Dec 10, 2022 •

edited

Loading

lmiller1990 Dec 11, 2022

ryanthemanuel Dec 12, 2022

lmiller1990 Dec 11, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022

ryanthemanuel Dec 12, 2022

emilyrohrbough Dec 12, 2022 •

edited

Loading

ryanthemanuel Dec 12, 2022

fix: add column, line, and method check to integrity check #25094

fix: add column, line, and method check to integrity check #25094

Conversation

ryanthemanuel commented Dec 10, 2022

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Dec 10, 2022

cypress bot commented Dec 10, 2022 • edited Loading

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Dec 10, 2022 •

edited

Loading

emilyrohrbough Dec 12, 2022 •

edited

Loading