Fix Website Breakage Reports by allowing unescaped commas in URL query #498
Conversation
| ) | ||
| } | ||
|
|
||
| func parameters(from websiteBreakage: WebsiteBreakage) -> [String: String] { |
There was a problem hiding this comment.
Optional: This method could be private (Helps readers with the separation of class API and class internal logic)
There was a problem hiding this comment.
The reason why this method remained public is that it's used in unit tests. But it actually has no dependency on WebsiteBreakageSender, so I'll refactor it into a computed property on WebsiteBreakage called requestParameters. It should make things more clear.
| urlParametersRemoved: Bool, | ||
| manufacturer: String = "Apple" | ||
| ) { | ||
| var siteUrlComponents = URLComponents(string: siteUrlString) |
There was a problem hiding this comment.
The breakage form has a URL field which shows what URL are we going to submit. The trimming of query and fragment is in the layer above - in the view controller layer. I don't recommend doing the trimming on two places in case something changes. I you want to be sure things are trimmed, this can be just an assertion.
There was a problem hiding this comment.
You're obviously right, I missed that part of code. Will remove it.
| } | ||
|
|
||
| static let allowedQueryReservedCharacters = CharacterSet(charactersIn: ",") |
There was a problem hiding this comment.
Optional: It is a good practice to have static variables or methods on top
DuckDuckGo/Feedback and Breakage/Model/WebsiteBreakageSender.swift
Outdated
Show resolved
Hide resolved
Task/Issue URL: https://app.asana.com/0/1163321984198618/1202033667108435/f
Tech Design URL:
CC:
Description:
Website Breakage reporting uses comma (a reserved character as described by RFC 3986) to separate blocked trackers domains in a single URL query parameter. Since we upgraded URL.addParameter to percent-encode all reserved characters, macOS website breakage reports got malformed, because of percent-encoded commas.
This change utilizes an update to
URL.addParameter(added in BSK) that allows to explicitly specify characters that are excluded from percent encoding. Then, inWebsiteBreakageSender, a comma is added as allowed character, to fix the reports.Adding this new API required updating
APIRequestandPixel's code, but since the allowed characters set defaults toniland is an opt-in feature, no API request other than Website Breakage should change.I also added the initial version of Website Breakage reference tests, which checks contents of URL requests sent via Pixel. Here is the reference JSON file I was inspired by. Since it turned out that our breakage reporting mechanism is a little bit off relative to the reference, I made the following changes:
gpcflagmanufacturer- it defaults to "Apple" anyway, but reference tests JSON contains different values there so I enabled it for the future when we'll have automatic tests working on the JSON file.Steps to test this PR:
Testing checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM