Fix: Frame Injection (azul-private#12) #5035

dsotirho-ucsc · 2023-03-09T01:09:53Z

Connected issues: DataBiosphere/azul-private#12

Checklist

Author

PR is a draft
Target branch is develop
Name of PR branch matches issues/<GitHub handle of author>/<issue#>-<slug>
PR title references all connected issues
PR title matches¹ that of a connected issue _{or comment in PR explains why they're different}
For each connected issue, there is at least one commit whose title references that issue
PR is connected to all connected issues via ZenHub
PR description links to connected issues
Added partial label to PR _{or this PR completely resolves all connected issues}

¹ when the issue title describes a problem, the corresponding PR
title is Fix: followed by the issue title

Author (reindex, API changes)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}
Added a (compatible changes) or A (incompatible ones) tag to commit title _{or this PR does not modify the Azul service API}
Added API label to connected issues _{or this PR does not modify the Azul service API}

Author (chains)

This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added base label to the blocking PR _{or this PR is not chained to another PR}
Added chained label to this PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}

Author (operator tasks)

Added checklist items for additional operator tasks _{or this PR does not require additional tasks}

Author (hotfixes)

Added F tag to main commit title _{or this PR does not include permanent fix for a temporary hotfix}
Reverted the temporary hotfixes for any connected issues _{or the prod branch has no temporary hotfixes for any connected issues}

Author (before every review)

Rebased PR branch on develop, squashed old fixups
Ran make requirements_update _{or this PR does not touch requirements*.txt, common.mk, Makefile and Dockerfile}
Added R tag to commit title _{or this PR does not touch requirements*.txt}
Added reqs label to PR _{or this PR does not touch requirements*.txt}
make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}

Peer reviewer (after requesting changes)

Uncheck the Author (before every review) checklists.

Peer reviewer (after approval)

PR is not a draft
Ticket is in Review requested column
Requested review from primary reviewer
Assigned PR to primary reviewer

Primary reviewer (after requesting changes)

Uncheck the before every review checklists. Update the N reviews label.

Primary reviewer (after approval)

Actually approved the PR
Labeled connected issues as demo or no demo
Commented on connected issues about demo expectations _{or all connected issues are labeled no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
N reviews label is accurate
Moved ticket to Approved column
Assigned PR to current operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

¹ When pushing the merge commit is skipped due to the PR being
labelled no sandbox, the next build triggered by a PR whose merge commit is
pushed determines this checklist item.

Operator (reindex)

Deleted unreferenced indices in dev _{or this PR does not remove catalogs or otherwise causes unreferenced indices}
Deleted unreferenced indices in anvildev _{or this PR does not remove catalogs or otherwise causes unreferenced indices}
Started reindex in dev _{or this PR does not require reindexing}
Started reindex in anvildev _{or this PR does not require reindexing}
Checked for and triaged indexing failures in dev _{or this PR does not require reindexing}
Checked for and triaged indexing failures in anvildev _{or this PR does not require reindexing}
Emptied fail queues in dev deployment _{or this PR does not require reindexing}
Emptied fail queues in anvildev deployment _{or this PR does not require reindexing}

Operator

Unassigned PR

Shorthand for review comments

L line is too long
W line wrapping is wrong
Q bad quotes
F other formatting problem

coveralls · 2023-03-09T01:36:08Z

Coverage: 84.737% (+0.1%) from 84.627% when pulling d658544 on issues/danielsotirhos/12-html-escape-error-response into e85ce07 on develop.

codecov · 2023-03-09T01:39:18Z

Codecov Report

Merging #5035 (cf2a871) into develop (e85ce07) will increase coverage by 0.11%.
The diff coverage is 99.27%.

❗ Current head cf2a871 differs from pull request most recent head d658544. Consider uploading reports for the commit d658544 to get more accurate results

@@             Coverage Diff             @@
##           develop    #5035      +/-   ##
===========================================
+ Coverage    84.60%   84.71%   +0.11%     
===========================================
  Files          148      149       +1     
  Lines        18060    18197     +137     
===========================================
+ Hits         15280    15416     +136     
- Misses        2780     2781       +1

Impacted Files	Coverage Δ
test/test_content_type.py	`99.16% <99.16%> (ø)`
src/azul/chalice.py	`89.74% <100.00%> (+0.70%)`	⬆️
test/app_test_case.py	`89.55% <100.00%> (+0.32%)`	⬆️

nadove-ucsc · 2023-03-10T00:58:42Z

src/azul/chalice.py

@@ -116,6 +121,21 @@ def patched_event_source_handler(self_, event, context):
        if old_handler.__code__ != patched_event_source_handler.__code__:
            chalice.app.EventSourceHandler.__call__ = patched_event_source_handler

+    def _wrapping_middleware(self, event, get_response):


More descriptive name

Suggested change

def _wrapping_middleware(self, event, get_response):

def _error_wrapping_middleware(self, event, get_response):

nadove-ucsc · 2023-03-10T01:11:33Z

src/azul/chalice.py

+            parsed = parse_accept_header(event.headers.get('accept'))
+            text_html = parsed.find('text/html')
+            star_star = parsed.find('*/*')
+            if -1 < text_html and (star_star == -1 or text_html < star_star):


I think it's more conventional to write the operands in this order. I certainly find it easier to read this way.

Suggested change

if -1 < text_html and (star_star == -1 or text_html < star_star):

if text_html > -1 and (star_star == -1 or text_html < star_star):

test/service/test_response.py

nadove-ucsc

Looks good 👍

hannes-ucsc

I'd like to see the tests prove that the original body is HTML-escaped. That's what's ultimately behind all this.

hannes-ucsc · 2023-03-11T21:19:27Z

src/azul/chalice.py

@@ -116,6 +121,21 @@ def patched_event_source_handler(self_, event, context):
        if old_handler.__code__ != patched_event_source_handler.__code__:
            chalice.app.EventSourceHandler.__call__ = patched_event_source_handler

+    def _error_wrapping_middleware(self, event, get_response):
+        response = get_response(event)
+        if response.status_code >= 400:


I specified a different condition in the ticket. Please fix or explain the deviation.

hannes-ucsc · 2023-03-11T21:27:36Z

src/azul/chalice.py

+            parsed = parse_accept_header(event.headers.get('accept'))
+            text_html = parsed.find('text/html')
+            star_star = parsed.find('*/*')
+            if text_html > -1 and (star_star == -1 or text_html < star_star):


Suggested change

if text_html > -1 and (star_star == -1 or text_html < star_star):

if 0 <= text_html and (star_star < 0 or text_html < star_star):

hannes-ucsc · 2023-03-11T21:29:40Z

src/azul/chalice.py

+                response.body = (
+                    '<html>'
+                    f'<head>Status {response.status_code}</head>'
+                    f'<body><pre>{html.escape(str(response.body), quote=False)}</pre></body>'


Explain use of { and str. What's the type of response.body?

hannes-ucsc · 2023-03-11T21:31:22Z

test/service/test_response.py

+        unescaped = json.dumps({
+            'Code': 'NotFoundError',
+            'Message': "Unable to find file 'foo', version None in catalog 'test'"
+        }, separators=(',', ':'))
+        escaped = (
+            "<html><head>Status 404</head><body><pre>{"
+            "'Code': 'NotFoundError', "
+            "'Message': \"Unable to find file 'foo', version None in catalog 'test'\""
+            "}</pre></body></html>"
+        )


This is all very hard to parse and understand. Consider triple-quoted string and textwrap.dedent.

Also don't mix serialized and deserialized literals. Makes it even harder to examine for correctness.

hannes-ucsc · 2023-03-11T21:31:55Z

test/service/test_response.py

+            ('text/html,*/*', True),
+        ]:
+            headers = {'accept': accept}
+            with self.subTest(headers=headers):


Suggested change

with self.subTest(headers=headers):

with self.subTest(accept=accept):

and swap lines

hannes-ucsc · 2023-04-17T23:35:32Z

test/test_content_type.py

+        def route():
+            return '<script />'
+
+        # noinspection PyUnusedLocal


Index: test/test_content_type.py IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/test/test_content_type.py b/test/test_content_type.py --- a/test/test_content_type.py (revision 145cc0157017310efbc5cbc6dbbfc973970962a5) +++ b/test/test_content_type.py (date 1681774413142) @@ -106,8 +106,7 @@ def route(): return '<script />' - # noinspection PyUnusedLocal - def expected(debug: bool, wrapped: bool) -> Tuple[str, str]: + def expected(_debug: bool, _wrapped: bool) -> tuple[str, str]: text = '<script />' content_type = 'application/json' return text, content_type

https://stackoverflow.com/a/49447358/4171119 is what we usually do in this case.

tuple is now generic.

hannes-ucsc · 2023-04-20T01:12:01Z

test/test_content_type.py

+            raise ChaliceUnhandledError('<script />')
+
+        def expected(_debug: bool, _wrapped: bool) -> tuple[str, str]:
+            if _debug:


Only use _ for unused parameters.

hannes-ucsc · 2023-04-24T19:18:04Z

src/azul/chalice.py

+        those do prefer `text/html`.
+        """
+        response = get_response(event)
+        ct_key = only(key for key in response.headers


Wrap all or None.

We typically use k for key and then you won't need to wrap.

github-actions bot added the orange [process] Done by the Azul team label Mar 9, 2023

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch from 3b279b1 to 9521984 Compare March 9, 2023 16:56

dsotirho-ucsc requested a review from nadove-ucsc March 9, 2023 22:48

dsotirho-ucsc assigned nadove-ucsc Mar 9, 2023

nadove-ucsc requested changes Mar 10, 2023

View reviewed changes

nadove-ucsc removed their assignment Mar 10, 2023

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch from 9521984 to 01c25a7 Compare March 10, 2023 19:00

dsotirho-ucsc requested a review from nadove-ucsc March 10, 2023 19:22

dsotirho-ucsc assigned nadove-ucsc Mar 10, 2023

nadove-ucsc previously approved these changes Mar 10, 2023

View reviewed changes

nadove-ucsc removed their assignment Mar 10, 2023

nadove-ucsc marked this pull request as ready for review March 10, 2023 19:36

nadove-ucsc requested a review from hannes-ucsc as a code owner March 10, 2023 19:36

nadove-ucsc assigned hannes-ucsc Mar 10, 2023

hannes-ucsc requested changes Mar 11, 2023

View reviewed changes

hannes-ucsc added the 1 review [process] Lead requested changes once label Mar 11, 2023

hannes-ucsc removed their assignment Mar 11, 2023

hannes-ucsc force-pushed the develop branch from 9c07f32 to 317e856 Compare March 12, 2023 19:03

dsotirho-ucsc dismissed nadove-ucsc’s stale review via b2753ca March 13, 2023 23:56

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch 4 times, most recently from 9a68982 to a935cdd Compare March 20, 2023 18:29

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch 5 times, most recently from 4a68f38 to bb0e83e Compare March 22, 2023 00:29

hannes-ucsc requested changes Apr 17, 2023

View reviewed changes

hannes-ucsc added 4+ reviews [process] Lead requested changes four times or more and removed 3 reviews [process] Lead requested changes thrice labels Apr 17, 2023

hannes-ucsc removed their assignment Apr 17, 2023

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch 2 times, most recently from d575b28 to 77544d6 Compare April 18, 2023 01:50

dsotirho-ucsc requested a review from hannes-ucsc April 18, 2023 16:02

dsotirho-ucsc assigned hannes-ucsc Apr 18, 2023

hannes-ucsc requested changes Apr 20, 2023

View reviewed changes

hannes-ucsc removed their assignment Apr 20, 2023

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch from 77544d6 to 81b4367 Compare April 20, 2023 16:29

dsotirho-ucsc requested a review from hannes-ucsc April 20, 2023 17:07

dsotirho-ucsc assigned hannes-ucsc Apr 20, 2023

hannes-ucsc requested changes Apr 24, 2023

View reviewed changes

hannes-ucsc removed their assignment Apr 24, 2023

dsotirho-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch from 81b4367 to cf2a871 Compare April 24, 2023 20:12

dsotirho-ucsc requested a review from hannes-ucsc April 24, 2023 22:14

dsotirho-ucsc assigned hannes-ucsc Apr 24, 2023

hannes-ucsc previously approved these changes Apr 25, 2023

View reviewed changes

hannes-ucsc assigned achave11-ucsc and unassigned hannes-ucsc Apr 25, 2023

Fix: Frame Injection (azul-private#12)

d658544

achave11-ucsc dismissed hannes-ucsc’s stale review via d658544 April 26, 2023 18:22

achave11-ucsc force-pushed the issues/danielsotirhos/12-html-escape-error-response branch from cf2a871 to d658544 Compare April 26, 2023 18:22

achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 26, 2023

hannes-ucsc approved these changes Apr 26, 2023

View reviewed changes

achave11-ucsc merged commit ca7793d into develop Apr 26, 2023

achave11-ucsc deleted the issues/danielsotirhos/12-html-escape-error-response branch April 26, 2023 22:52

achave11-ucsc removed their assignment Apr 26, 2023

dsotirho-ucsc mentioned this pull request May 12, 2023

HTML wrapping middleware is redundant with WAF enabled #5197

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Frame Injection (azul-private#12) #5035

Fix: Frame Injection (azul-private#12) #5035

dsotirho-ucsc commented Mar 9, 2023 •

edited by achave11-ucsc

Loading

coveralls commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading

nadove-ucsc Mar 10, 2023

nadove-ucsc Mar 10, 2023

nadove-ucsc left a comment

hannes-ucsc left a comment

hannes-ucsc Mar 11, 2023

hannes-ucsc Mar 11, 2023

hannes-ucsc Mar 11, 2023

hannes-ucsc Mar 11, 2023

hannes-ucsc Mar 11, 2023

hannes-ucsc Apr 17, 2023

hannes-ucsc Apr 20, 2023

hannes-ucsc Apr 24, 2023

hannes-ucsc Apr 24, 2023

	def _wrapping_middleware(self, event, get_response):
	def _error_wrapping_middleware(self, event, get_response):

	if -1 < text_html and (star_star == -1 or text_html < star_star):
	if text_html > -1 and (star_star == -1 or text_html < star_star):

	if text_html > -1 and (star_star == -1 or text_html < star_star):
	if 0 <= text_html and (star_star < 0 or text_html < star_star):

	with self.subTest(headers=headers):
	with self.subTest(accept=accept):

Fix: Frame Injection (azul-private#12) #5035

Fix: Frame Injection (azul-private#12) #5035

Conversation

dsotirho-ucsc commented Mar 9, 2023 • edited by achave11-ucsc Loading

Checklist

Author

Author (reindex, API changes)

Author (chains)

Author (upgrading)

Author (operator tasks)

Author (hotfixes)

Author (before every review)

Peer reviewer (after requesting changes)

Peer reviewer (after approval)

Primary reviewer (after requesting changes)

Primary reviewer (after approval)

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Shorthand for review comments

coveralls commented Mar 9, 2023 • edited Loading

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadove-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Mar 9, 2023 •

edited by achave11-ucsc

Loading

coveralls commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading