Skip to content

Conversation

@cuba
Copy link
Contributor

@cuba cuba commented Jul 31, 2024

Resolves brave/brave-browser#40173

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@cuba cuba added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Jul 31, 2024
@cuba cuba force-pushed the js/freeze-js-apis branch from 4e66388 to 1a60d35 Compare July 31, 2024 19:14
@cuba cuba marked this pull request as ready for review August 2, 2024 15:07
@cuba cuba requested a review from a team as a code owner August 2, 2024 15:07
@kylehickinson kylehickinson requested a review from Brandon-T August 2, 2024 15:13
return value;
};

/// Secure URL object
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to call secure object on these, just like on line 133.
Otherwise these do nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well they keep people from changing the definition of URL to a string. Which is essentially what I want here. But calling secureObject on these results in the following errors:

TypeError: ProxyObject is not a constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with it, but I believe they can still technically change the original definition by re-defining the object's functions or w/e.

I'll see if I can add a proxy constructor trap. If it works then we can add it to this PR, if not, it's okay and I'll approve this.

Copy link
Contributor Author

@cuba cuba Aug 9, 2024

Choose a reason for hiding this comment

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

just have to be careful here. some of these are used in our farbling script so these secure objects some will become available to the web page itself. Perhaps I shouldn't use these there but instead use window.. Or perhaps we shouldn't secure those specific APIs

@cuba cuba force-pushed the js/freeze-js-apis branch from 1a60d35 to 1e58b77 Compare August 9, 2024 17:31
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/skip-android Do not run CI builds for Android CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-windows-x64 Do not run CI builds for Windows x64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS adblocking script API's are not frozen

5 participants