Page MenuHomePhabricator

[blob] Block reserved and offensive links from being created
ClosedPublic

Authored by tomek on Feb 9 2024, 9:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:39 PM
Unknown Object (File)
Sat, Nov 23, 6:08 PM
Unknown Object (File)
Thu, Nov 14, 3:41 AM
Unknown Object (File)
Fri, Nov 8, 10:03 PM
Unknown Object (File)
Fri, Nov 8, 7:14 PM
Unknown Object (File)
Fri, Nov 8, 7:14 PM
Unknown Object (File)
Fri, Nov 8, 7:14 PM
Unknown Object (File)
Fri, Nov 8, 7:14 PM
Subscribers

Details

Summary

Copy a list of reserved usernames from keyserver - the same list is used when handling reserved links on keyserver. The same approach of copying this list is used in Identity service.

Copy a list of offensive words that is used in bad-words library. The list is modified from being an object to a plain list. The license (which is also included) allows us to do it.

Use the two lists to filter out the links that we don't want to be created.

Depends on D10992

Test Plan

Run the service locally. Modify keyserver code to not check if a link is reserved or offensive.
Check if:

  1. Creating a reserved link is blocked
  2. Creating an offensive link is blocked
  3. Creating a link that isn't contain in these lists succeeds

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

services/blob/src/service.rs
153 ↗(On Diff #36944)

I have to review how the library uses the list - instead of simply checking for equality it uses the items as regexes, and we probably need to do the same here.

tomek requested review of this revision.Feb 9 2024, 9:47 AM
services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

Do we really need to copy-paste this? Can't we share the same JSON file across all of the places we use this?

services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

We can move this to shared and use it in both identity and here. Just need to remember to add it to both Dockerfiles

services/blob/src/service.rs
131 ↗(On Diff #36944)

Do you think we should also check holders for offensive words? in assign_holder(). They're not used for invite links, but just in case

144–159 ↗(On Diff #36944)

I'd extract body if this if { .. } to a separate helper function fn validate_invite_link_blob_hash(blob_hash: &str) -> Result<(), BlobServiceError> and call it here validate_invite_link_blob_hash(&blob_hash)?;

153 ↗(On Diff #36944)

Yeah, this code here only checks if invite_secret === "some bad word" (equality check)

services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

@bartek is there a way of sharing it also with the keyserver?

services/blob/src/service.rs
131 ↗(On Diff #36944)

I'm not sure if it makes sense. For invite link hash it is a good idea because these are accessible as URLs comm.app/invite/${secret} and we don't want these to be offensive. The holders aren't visible at all, so it should be fine to not have any additional validation for them. @ashoat what do you think?

services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

We could potentially use a relative symlink

services/blob/src/service.rs
131 ↗(On Diff #36944)

That reasoning makes sense to me

tomek marked 4 inline comments as done.

Check regexes and refactor

services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

I think it would be more convenient to handle this as a separate diff - going to put up a followup

tomek added inline comments.
lib/facts/blob-service.js
37 ↗(On Diff #36996)

This shouldn't be included in this diff

Revert unnecessary changes

This revision is now accepted and ready to land.Feb 13 2024, 5:14 AM
services/blob/reserved_usernames.json
1 ↗(On Diff #36944)

Created a task https://linear.app/comm/issue/ENG-6780/share-reserved-usernames-between-services and a diff https://phab.comm.dev/D11051 which makes the file shared between services. Sharing this file between keyserver and services is more complicated - created a task for it https://linear.app/comm/issue/ENG-6781/share-reserved-usernames-between-services-and-lib.