Redact GutenbergKit requests - #25854
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33546 | |
| Version | PR #25854 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 33e1a66 | |
| Installation URL | 54patdh22ap5o |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33546 | |
| Version | PR #25854 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 33e1a66 | |
| Installation URL | 42b3fuqd2602g |
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Verified sensitive headers are now redacted for GBK requests. Changes look good overall. I left a few suggestions for consideration.
| _ viewController: GutenbergKit.EditorViewController, | ||
| didLogNetworkRequest request: GutenbergKit.RecordedNetworkRequest | ||
| ) { | ||
| guard ExtensiveLogging.enabled, let url = URL(string: request.url) else { |
There was a problem hiding this comment.
Was moving the ExtensiveLogging.enabled check into storeRequest intentional? Every WebView request in the editor now builds a URLRequest and HTTPURLResponse and UTF-8-encodes both bodies before storeRequest checks logger != nil and discards it — so logging-disabled sessions (most users) pay that cost on every request. Could the guard come back at the top here, with the one inside storeRequest kept as the safety net?
There was a problem hiding this comment.
Good point. I have brought it back.
| guard logger != nil else { return } | ||
|
|
||
| var request = request | ||
| request.allHTTPHeaderFields = Self.redactedHeaders(request.allHTTPHeaderFields) |
There was a problem hiding this comment.
This may be low-risk given we do not utilize the sensitiveQueryItems and sensitiveDataFields configuration options today, but noting that if they are added in the future, it seems this custom handler will not apply those. So, it's possible that future sensitive information is redacted from the NetworkLogger path, but remains present in this GBK-centric path.
Options might be a test asserting both paths redact equivalently to flag any future divergence, or exploring if storeRequest can read from Network.Configuration rather than Self.sensitiveHeaders so the divergence can't happen at all. WDYT?
There was a problem hiding this comment.
I don't think we put have sensitive data in URL. "read from Network.Configuration rather than Self.sensitiveHeaders" seems cleaner. I'll look into it later.
* Redact GutenbergKit requests * Add `ExtensiveLogging.enabled` check back
* Share Gallery media library handling between GutenbergKit editors (wordpress-mobile#25855) * Allow Gutenberg media picker to use a blog * Share Gallery media library handling between GutenbergKit editors * Redact GutenbergKit requests (wordpress-mobile#25854) * Redact GutenbergKit requests * Add `ExtensiveLogging.enabled` check back * Update app translations – `Localizable.strings` * Update plural translations from GlotPress * Update WordPress metadata translations * Update Jetpack metadata translations * Bump version number --------- Co-authored-by: Tony Li <tony.li@automattic.com>
4 participants
Description
Fixes https://linear.app/a8c/issue/CMM-2191.
The GBK requests went directly to the log store, without redacting sensitive headers.