Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add dict instance check before accessing it #15944

Closed
wants to merge 14 commits into from
Closed

Add dict instance check before accessing it #15944

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2d99953
fix: add dict instance check before accessing it
Jul 16, 2023
dc16c05
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 16, 2023
0560a7b
fix: fixing a typo of a method name isinstance my last commit
Jul 16, 2023
6fc5e78
fix: Added changelog file for this fix
Jul 16, 2023
fe888d2
Added . at the end of line in changelog file
Jul 17, 2023
246d6d5
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 17, 2023
9f0894f
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 24, 2023
3127227
Throwing error if client data is not dict
Jul 24, 2023
ec1c23a
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 25, 2023
5b19acb
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 25, 2023
ad20c6c
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 28, 2023
2f43ac7
fix code format style
Jul 28, 2023
2a2eeb6
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Jul 31, 2023
349bd24
Merge branch 'develop' into bugfix/issue-15871/500-error-uia-request
rajuniit Sep 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15944.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add dic instance check before accessing auth session.
5 changes: 5 additions & 0 deletions synapse/handlers/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,11 @@ async def check_ui_auth(

sid: Optional[str] = None
authdict = clientdict.pop("auth", {})
if not isinstance(authdict, dict):
raise SynapseError(
400,
"Interactive auth not yet complete. Client data is not dictionary.",
)
Comment on lines +484 to +488
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a chance to look deeper at this; check_ui_auth has 3 callers:

  • validate_user_via_ui_auth, this has a handful of callers.
  • PasswordRestServlet.on_POST
  • RegisterRestServlet.on_POST

Some of these properly validate that auth is None or a dictionary. Others don't have validation (see #13147). I'm wondering if we should do validation up front instead of at this stage.

@DMRobertson It looks like you started adding the UI Auth validation of REST endpoints. Did you have a plan here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have a specific plan, I'm afraid. I vaguely remember UI auth being confusing when I looked it it, but that was about a year ago.

My general plan was to do as much validation as possible in the rest layer, and pass a parsed Pydantic model instance down to the handlers. The two can be done separately, which makes it easier to validate one endpoint at a time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an vaguely open question about how we should be using Pydantic going forward. It has just made a 2.x release with a strict mode built-in (no more check_pydantic_models.py) and performance improvements. There are some breaking changes though, plus some of our packagers are presumably going to be shipping Pydantic 1.x for some time. #15979 has some more detailed discussion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @clokep @DMRobertson, was some decision made on Pydantic? What should we do here for now?

If this function is called from every rest endpoint at the beginning should we keep it here, or move it upfront to rest layers which are using this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prajjawal05 I think adding the validation would be a good step forward. I don't agree there's an open question. Today we're using pydantic 1.x and should not block good work because we might one-day update to pydantic 2.x.

if "session" in authdict:
sid = authdict["session"]

Expand Down