Bug 222668 – [LayoutTests] Convert http/tests/security convert PHP to Python

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Gambrell]

Replacing PHP with equivalent Python CGI scripts

[Radar WebKit Bug Importer]

<rdar://problem/74993152>

[Chris Gambrell]

Created attachment 422145 [details]
Patch

[Chris Gambrell]

Created attachment 422248 [details]
Patch

[Chris Gambrell]

Created attachment 422263 [details]
Patch

[Chris Gambrell]

Created attachment 422382 [details]
Patch

[Chris Gambrell]

Created attachment 422411 [details]
Patch

[Chris Gambrell]

Created attachment 422432 [details]
Patch

[Chris Gambrell]

Created attachment 422498 [details]
Patch

[Chris Gambrell]

Created attachment 422541 [details]
Patch

[Chris Gambrell]

Created attachment 422585 [details]
Patch

[Chris Gambrell]

Created attachment 422596 [details]
Patch

[Chris Gambrell]

Created attachment 422652 [details]
Patch

[Jonathan Bedard]

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

[Chris Gambrell]

Created attachment 422786 [details]
Patch

[Chris Gambrell]

Created attachment 422787 [details]
Patch

[Chris Gambrell]

Created attachment 422799 [details]
Patch

[Jonathan Bedard]

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?

[Jonathan Bedard]

Comment on attachment 422799 [details]
Patch

The file I pointed out is going to be reserved for a separate change.

[EWS]

Committed r274244: <https://commits.webkit.org/r274244>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422799 [details].

[Chris Gambrell]

Reopening for the PHP files that were left

[Chris Gambrell]

Created attachment 425551 [details]
Patch

[Chris Gambrell]

Created attachment 425625 [details]
Patch

[Chris Gambrell]

Created attachment 425629 [details]
Patch

[Chris Gambrell]

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

[Chris Gambrell]

Created attachment 425639 [details]
Patch

[Chris Gambrell]

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

[Chris Gambrell]

Created attachment 425743 [details]
Patch

[Jonathan Bedard]

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

[Chris Gambrell]

Created attachment 425787 [details]
Patch

[Chris Gambrell]

Created attachment 425810 [details]
Patch

[EWS]

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].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 224476

[Chris Gambrell]

Created attachment 425875 [details]
Patch

[Chris Gambrell]

Created attachment 425908 [details]
Patch

[Jonathan Bedard]

Comment on attachment 425908 [details]
Patch

Now that Kate's change has landed, let's try this again.

[EWS]

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].