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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,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.

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