Bug 222668 - [LayoutTests] Convert http/tests/security convert PHP to Python
Summary: [LayoutTests] Convert http/tests/security convert PHP to Python
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Gambrell
URL:
Keywords: InRadar
Depends on: 224476
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-03 10:58 PST by Chris Gambrell
Modified: 2021-04-13 16:22 PDT (History)
13 users (show)

See Also:


Attachments
Patch (134.10 KB, patch)
2021-03-03 13:38 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (293.79 KB, patch)
2021-03-04 09:56 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (332.96 KB, patch)
2021-03-04 11:25 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (409.69 KB, patch)
2021-03-05 10:40 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (583.12 KB, patch)
2021-03-05 13:51 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (586.53 KB, patch)
2021-03-05 15:14 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (590.47 KB, patch)
2021-03-06 10:36 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (658.08 KB, patch)
2021-03-07 18:36 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (657.03 KB, patch)
2021-03-08 10:56 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (657.10 KB, patch)
2021-03-08 11:54 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (659.95 KB, patch)
2021-03-08 18:49 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (658.87 KB, patch)
2021-03-09 18:19 PST, Chris Gambrell
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (653.08 KB, patch)
2021-03-09 19:52 PST, Chris Gambrell
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (655.98 KB, patch)
2021-03-09 21:51 PST, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (336.18 KB, patch)
2021-04-08 15:44 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (338.64 KB, patch)
2021-04-09 09:08 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (359.86 KB, patch)
2021-04-09 10:20 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (338.72 KB, patch)
2021-04-09 12:27 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (338.66 KB, patch)
2021-04-12 07:11 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (361.18 KB, patch)
2021-04-12 14:08 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (339.58 KB, patch)
2021-04-12 16:56 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff
Patch (340.63 KB, patch)
2021-04-13 08:43 PDT, Chris Gambrell
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (340.65 KB, patch)
2021-04-13 13:47 PDT, Chris Gambrell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Gambrell 2021-03-03 10:58:02 PST
Replacing PHP with equivalent Python CGI scripts
Comment 1 Radar WebKit Bug Importer 2021-03-03 10:58:23 PST
<rdar://problem/74993152>
Comment 2 Chris Gambrell 2021-03-03 13:38:13 PST
Created attachment 422145 [details]
Patch
Comment 3 Chris Gambrell 2021-03-04 09:56:23 PST
Created attachment 422248 [details]
Patch
Comment 4 Chris Gambrell 2021-03-04 11:25:15 PST
Created attachment 422263 [details]
Patch
Comment 5 Chris Gambrell 2021-03-05 10:40:50 PST
Created attachment 422382 [details]
Patch
Comment 6 Chris Gambrell 2021-03-05 13:51:01 PST
Created attachment 422411 [details]
Patch
Comment 7 Chris Gambrell 2021-03-05 15:14:33 PST
Created attachment 422432 [details]
Patch
Comment 8 Chris Gambrell 2021-03-06 10:36:33 PST
Created attachment 422498 [details]
Patch
Comment 9 Chris Gambrell 2021-03-07 18:36:58 PST
Created attachment 422541 [details]
Patch
Comment 10 Chris Gambrell 2021-03-08 10:56:47 PST
Created attachment 422585 [details]
Patch
Comment 11 Chris Gambrell 2021-03-08 11:54:08 PST
Created attachment 422596 [details]
Patch
Comment 12 Chris Gambrell 2021-03-08 18:49:39 PST
Created attachment 422652 [details]
Patch
Comment 13 Jonathan Bedard 2021-03-09 09:10:23 PST
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
Comment 14 Chris Gambrell 2021-03-09 18:19:10 PST
Created attachment 422786 [details]
Patch
Comment 15 Chris Gambrell 2021-03-09 19:52:13 PST
Created attachment 422787 [details]
Patch
Comment 16 Chris Gambrell 2021-03-09 21:51:45 PST
Created attachment 422799 [details]
Patch
Comment 17 Jonathan Bedard 2021-03-10 13:35:35 PST
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 18 Jonathan Bedard 2021-03-10 14:58:52 PST
Comment on attachment 422799 [details]
Patch

The file I pointed out is going to be reserved for a separate change.
Comment 19 EWS 2021-03-10 15:33:57 PST
Committed r274244: <https://commits.webkit.org/r274244>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422799 [details].
Comment 20 Chris Gambrell 2021-04-06 16:11:53 PDT
Reopening for the PHP files that were left
Comment 21 Chris Gambrell 2021-04-08 15:44:28 PDT
Created attachment 425551 [details]
Patch
Comment 22 Chris Gambrell 2021-04-09 09:08:35 PDT
Created attachment 425625 [details]
Patch
Comment 23 Chris Gambrell 2021-04-09 10:20:34 PDT
Created attachment 425629 [details]
Patch
Comment 24 Chris Gambrell 2021-04-09 10:21:37 PDT
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
Comment 25 Chris Gambrell 2021-04-09 12:27:34 PDT
Created attachment 425639 [details]
Patch
Comment 26 Chris Gambrell 2021-04-09 12:29:31 PDT
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
Comment 27 Chris Gambrell 2021-04-12 07:11:21 PDT
Created attachment 425743 [details]
Patch
Comment 28 Jonathan Bedard 2021-04-12 13:44:03 PDT
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
Comment 29 Chris Gambrell 2021-04-12 14:08:56 PDT
Created attachment 425787 [details]
Patch
Comment 30 Chris Gambrell 2021-04-12 16:56:26 PDT
Created attachment 425810 [details]
Patch
Comment 31 EWS 2021-04-12 17:33:09 PDT
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].
Comment 32 WebKit Commit Bot 2021-04-13 04:00:50 PDT
Re-opened since this is blocked by bug 224476
Comment 33 Chris Gambrell 2021-04-13 08:43:25 PDT
Created attachment 425875 [details]
Patch
Comment 34 Chris Gambrell 2021-04-13 13:47:43 PDT
Created attachment 425908 [details]
Patch
Comment 35 Jonathan Bedard 2021-04-13 15:45:17 PDT
Comment on attachment 425908 [details]
Patch

Now that Kate's change has landed, let's try this again.
Comment 36 EWS 2021-04-13 16:22:29 PDT
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].