test: add authz unit tests

[xwid138]

When reading a path not starting with /pub/ Homeserver responds with Writing to directories other than '/pub/' is forbidden error message which is confusing for GET requests.

WARN request{method=GET uri="/test/hello.txt" version=HTTP/1.1}: pubky_homeserver::client_server::layers::authz: Writing to directories other than '/pub/' is forbidden: 8kcqwm7fw43j73jo4s4thzawzhjodc6zrn9cjyko4pzwxpkmwwey//test/hello.txt. Access forbidden

cargo run user get /test/hello.txt ./recovery.key
.
.
.
Error: Failed to get data

Caused by:
    0: Request failed: Server responded with an error: 403 Forbidden - Writing to directories other than '/pub/' is forbidden
    1: Server responded with an error: 403 Forbidden - Writing to directories other than '/pub/' is forbidden

After change:

Error: Failed to get data

Caused by:
    0: Request failed: Server responded with an error: 400 Bad Request - Invalid URL: Path must start with /pub/
    1: Server responded with an error: 400 Bad Request - Invalid URL: Path must start with /pub/

[SHAcollision]

Hey xwid138:fix-authz thank you for bringing this up and coming up with a fix. 🙌

I left a comment for a different take of this fix that could equally solve the problem you exposed without opening up some possible unsafe behaviour.

[SHAcollision]

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]

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]

Message update.

[SHAcollision]

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

[SHAcollision]

LGTM! Let me know if you'd like to add more to this PR ( OPTIONS and some tests) or if you would like do that on a new PR and we merge this message change.

Edit: looks like we need to adjust this test for the test suite to be happy

pubky-core/pubky-sdk/bindings/js/pkg/tests/storage.ts

Lines 163 to 187 in 9c99d07

	test("forbidden: writing outside /pub returns 403", async (t) => {
	const sdk = Pubky.testnet();
	const signer = sdk.signer(Keypair.random());
	const signupToken = await createSignupToken();
	const session = await signer.signup(HOMESERVER_PUBLICKEY, signupToken);
	const forbiddenPath = "/priv/example.com/arbitrary";
	try {
	await session.storage.putText(forbiddenPath as unknown as Path, "Hello");
	t.fail("putText to /priv should fail with 403");
	} catch (error) {
	assertPubkyError(t, error);
	t.equal(error.name, "RequestError", "mapped error name");
	t.equal(getStatusCode(error), 403, "status code 403");
	t.ok(
	String(error.message || "").includes(
	"Writing to directories other than '/pub/'",
	),
	"error message mentions /pub restriction",
	);
	}
	t.end();
	});

[xwid138]

Thanks for the review @SHAcollision .

I'll work on this PR and add mentioned changes. I will re-request a review once it's ready.