Summary: | [LayoutTests] Convert http/tests/security convert PHP to Python | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Gambrell <cgambrell> | ||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Chris Gambrell <cgambrell> | ||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, eric.carlson, ews-watchlist, glenn, hi, jbedard, jer.noble, mkwst, philipj, ryanhaddad, sergio, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=220749 https://bugs.webkit.org/show_bug.cgi?id=223079 https://bugs.webkit.org/show_bug.cgi?id=223978 https://bugs.webkit.org/show_bug.cgi?id=224498 |
||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 224476 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Gambrell
2021-03-03 10:58:02 PST
Created attachment 422145 [details]
Patch
Created attachment 422248 [details]
Patch
Created attachment 422263 [details]
Patch
Created attachment 422382 [details]
Patch
Created attachment 422411 [details]
Patch
Created attachment 422432 [details]
Patch
Created attachment 422498 [details]
Patch
Created attachment 422541 [details]
Patch
Created attachment 422585 [details]
Patch
Created attachment 422596 [details]
Patch
Created attachment 422652 [details]
Patch
Comment on attachment 422652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422652&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scriptnonce-allowed-by-legacy-enforced-policy-and-blocked-by-report-policy.py:6 > + 'Content-Security-Policy-Report-Only: script-src \'nonce-that-is-not-equal-to-dummy\' \'nonce-dump-as-text\'; report-uri ../resources/save-report.php?test=/security/contentSecurityPolicy/1.1/scriptnonce-allowed-by-legacy-enforced-policy-and-blocked-by-report-policy.py\r\n' Probably should do the same thing I did with redirect.php with save-report.php after this change is landed. > LayoutTests/http/tests/security/contentSecurityPolicy/resources/determine-content-security-policy-header.php:-10 > - $contentSecurityPolicy = stripslashes($contentSecurityPolicy); I don't see us considering the behavior of escaping the content security policy in Python > LayoutTests/http/tests/security/contentSecurityPolicy/resources/determine_content_security_policy_header.py:7 > +csp = parse_qs(os.environ.get('QUERY_STRING', ''), keep_blank_values=True).get('csp', [''])[0] I know PHP runs the file contents as it is imported, but I actually think we should put this into a function called "determine_content_security_policy_header()" and rename this file "to utils.py" and then everything that uses this should do: "from util import determine_content_security_policy_header" and then invoke the function. This is more Pythonic than relying on the import to run your code. > LayoutTests/http/tests/security/contentSecurityPolicy/resources/go-to-echo-report.py:17 > + ' window.location = "/security/contentSecurityPolicy/resources/echo-report.php?test={}";\n' Shouldn't this be echo-report.py? > LayoutTests/http/tests/security/mixedContent/resources/subresource2/protected-image.py:17 > + 'WWW-authenticate: Basic realm="{}"\r\n' We don't format this string > LayoutTests/http/tests/security/resources/cookie-protected-script.py:1 > +#!/usr/bin/env python3 I don't see the php file you are converting? Has it been removed? > LayoutTests/http/tests/security/resources/image-access-control.py:8 > +allowOrigin = query.get('allow', [''])[0] Nit: We try to avoid camelCase in Python Created attachment 422786 [details]
Patch
Created attachment 422787 [details]
Patch
Created attachment 422799 [details]
Patch
Comment on attachment 422799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422799&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/report-blocked-uri-and-do-not-follow-redirect-when-sending-report.py:21 > + ' window.location = "/security/contentSecurityPolicy/resources/echo-report.php";\n' What is our plan with this file? Comment on attachment 422799 [details]
Patch
The file I pointed out is going to be reserved for a separate change.
Committed r274244: <https://commits.webkit.org/r274244> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422799 [details]. Reopening for the PHP files that were left Created attachment 425551 [details]
Patch
Created attachment 425625 [details]
Patch
Created attachment 425629 [details]
Patch
Comment on attachment 425629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425629&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/resources/worker.php:-9 > - if (get_magic_quotes_gpc()) { urllib.parse.parse_qs should take care of this Created attachment 425639 [details]
Patch
Comment on attachment 425639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425639&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/resources/echo-report.py:9 > +http_root = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(os.path.dirname(file))))) Needs to match http/tests/security/contentSecurityPolicy/resources/save_report.py since they are sharing files > LayoutTests/http/tests/security/contentSecurityPolicy/resources/worker.php:-9 > - if (get_magic_quotes_gpc()) { urllib.parse.parse_qs will take care of this Created attachment 425743 [details]
Patch
Comment on attachment 425639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425639&action=review > LayoutTests/ChangeLog:19 > + (get_cookies): We have a complicated enough file here, the changelog should have a brief explanation of the changes to each function. > LayoutTests/http/tests/media/resources/serve_video.py:1 > +#!/usr/bin/env python3 A bit surprised to see this file in the security patch > LayoutTests/http/tests/security/401-logout/401-logout.py:33 > + ) Nit: We should add a newline before each elif at the top level >> LayoutTests/http/tests/security/contentSecurityPolicy/resources/echo-report.py:9 >> +http_root = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(os.path.dirname(file))))) > > Needs to match http/tests/security/contentSecurityPolicy/resources/save_report.py since they are sharing files We don't use this in a subsequent import, though. I don't think we need lines 8-10 at all. > LayoutTests/http/tests/security/contentSecurityPolicy/resources/report_file_path.py:10 > +sys.path.insert(0, http_root) I don't see us importing anything after this point > LayoutTests/http/tests/security/contentSecurityPolicy/resources/worker.py:29 > + print('''var id = 0; I think we need to do this with sys.stdout.write() and newlines, JavaScript's idioms are too close to Python's Created attachment 425787 [details]
Patch
Created attachment 425810 [details]
Patch
Committed r275849 (236414@main): <https://commits.webkit.org/236414@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425810 [details]. Re-opened since this is blocked by bug 224476 Created attachment 425875 [details]
Patch
Created attachment 425908 [details]
Patch
Comment on attachment 425908 [details]
Patch
Now that Kate's change has landed, let's try this again.
Committed r275917 (236481@main): <https://commits.webkit.org/236481@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425908 [details]. |