Skip to content

test: add authz unit tests #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xwid138 wants to merge 10 commits into
base: main
Choose a base branch
from
Open

test: add authz unit tests #296

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4c3d638
check HTTP method in authz in case of writing
xwid138 Jan 8, 2026
40d733f
update error msg
xwid138 Jan 9, 2026
080fcf2
Revert "check HTTP method in authz in case of writing"
xwid138 Jan 9, 2026
5ffc4c9
adjust msg
xwid138 Jan 9, 2026
48fa5c3
add idea to gitignore
xwid138 Jan 13, 2026
feff3d5
authz unit tests
xwid138 Jan 14, 2026
51ffd80
adjust sdk tests
xwid138 Jan 14, 2026
424aedd
remove comment
xwid138 Jan 14, 2026
e62e659
Merge branch 'main' into fix-authz
xwid138 Jan 14, 2026
c4d2632
fix tests after merge
xwid138 Jan 14, 2026
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
2 changes: 1 addition & 1 deletion pubky-homeserver/src/client_server/layers/authz.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ async fn authorize(
// if method == Method::GET {
// return Ok(());
// }
} else {
} else if method == Method::PUT {

@SHAcollision SHAcollision Jan 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could potentially allow other writing methods like DELETE into non /pub/ paths.

Feels like this whole conditional is hacky, but it's kindda correct "as is". So we might simply want to change L123 and L128 messages to "Access to non-/pub/ paths is forbidden” so it is accurate for reads too. What do you think?

@xwid138 xwid138 Jan 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's true - my bad.

I suggest adding some unit tests to authz, just to be safe. Wdyt ?

edit: should we allow for OPTIONS method as well in case of /pub/ path ? Only GET and HEADER are allowed.

@xwid138 xwid138 Jan 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Message update.

@SHAcollision SHAcollision Jan 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, we should allow OPTIONS too, and yes, this will greatly benefit from some tests 😅

tracing::warn!(
"Writing to directories other than '/pub/' is forbidden: {}/{}. Access forbidden",
public_key,
Expand Down
Loading