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

Fix secure coding error #2604

Merged
merged 4 commits into from
Apr 12, 2024
Merged

Fix secure coding error #2604

merged 4 commits into from
Apr 12, 2024

Conversation

Bunn
Copy link
Collaborator

@Bunn Bunn commented Apr 11, 2024

Task/Issue URL: https://app.asana.com/0/1203581873609357/1207056719863102/f

Description:
Fix issue sending XPC messages on error

Steps to test this PR:

  1. To make your life easier, delete all data brokers from the DBP package Resources/JSON and leave a single broker. Also, I recommend deleting the database on /Library/Group\ Containers/HKE973VLUW.com.duckduckgo.macos.browser.dbp*
  2. Change the broker in a way that it's going to force an error, for example, changing a selector to something that doesn't exist ("selector": ".search-item", -> "selector": ".potato-item",)
  3. Run the scan flow using the normal user flow making sure you're debugging the background agent
  4. Add a breakpoint on to make sure errors are returning
  5. Run the scan to completion, it shouldn't throw any crash or assertion

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 11, 2024
@Bunn Bunn changed the title WIP: Fix secure coding error Fix secure coding error Apr 11, 2024
@Bunn Bunn requested a review from THISISDINOSAUR April 11, 2024 18:10

public required init?(coder: NSCoder) {
oneTimeError = coder.decodeObject(forKey: NSSecureCodingKeys.oneTimeError) as? Error
operationErrors = coder.decodeObject(forKey: NSSecureCodingKeys.operationErrors) as? [Error]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should do something if these fail, but then also not sure if we can ever know since they're optional?
Would they ever fail? It feels like they shouldn't...

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://developer.apple.com/documentation/foundation/nssecurecoding

This bit specifically:
Screenshot 2024-04-11 at 9 11 21 PM

I've zero idea if this may break in some way... but if we want to adhere to secure coding strictly, we should decode using decodeObject(of:forKey).

Copy link
Contributor

@THISISDINOSAUR THISISDINOSAUR left a comment

Choose a reason for hiding this comment

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

Testing everything looks good to me, and logically this should work. The mistake is pretty obvious, with a clear fix. I also tested with nil/no errors and it worked.
Let's see if Diego has any nonobvious pitfalls though

@THISISDINOSAUR THISISDINOSAUR removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 11, 2024
oneTimeError = coder.decodeObject(forKey: NSSecureCodingKeys.oneTimeError) as? Error
operationErrors = coder.decodeObject(forKey: NSSecureCodingKeys.operationErrors) as? [Error]
oneTimeError = coder.decodeObject(of: NSError.self, forKey: NSSecureCodingKeys.oneTimeError)
operationErrors = coder.decodeArrayOfObjects(ofClass: NSError.self, forKey: NSSecureCodingKeys.operationErrors)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice improvement 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great to me.

@diegoreymendez diegoreymendez self-requested a review April 12, 2024 08:22
@Bunn Bunn merged commit 209a425 into main Apr 12, 2024
16 of 17 checks passed
@Bunn Bunn deleted the bunn/dbp/fix-secure-coding branch April 12, 2024 08:42
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