Use SHA256 to generate server IDs #437

matiasgarciaisaia · 2021-06-07T00:27:42Z

To have stable, shareable server URLs, we use a hash of domain+ApiKey instead of a (local) UUID for each server.

A server defined like this:

{
    "name": "Main server",
    "url": "https://doma.in",
    "apiKey": "09c972b7-506b-49f1-a19a-d729e22e599c"
  }

Will now always be at http(s)://SHLINK_WEB_CLIENT/server/10a665ba8a86d62bed0025f9d5bf2ba6f7c2f0a95c57b8e86ec2936af348e35e.

I'm marking this as draft because there's still a race condition in which users navigating to a server URL for the first time get a "server not found" error because the page hasn't loaded the servers.json file yet - and I want to address that here.

See #435

To have stable, shareable server URLs, we use a hash of domain+ApiKey instead of a (local) UUID for each server. See shlinkio#435

github-actions · 2021-06-07T00:29:56Z

Preview environment

https://shlinkio.github.io/shlink-web-client/feature-stable-server-id/

matiasgarciaisaia · 2021-06-07T00:29:58Z

src/servers/CreateServer.tsx


-    createServer({ ...serverData, id });
-    push(`/server/${id}`);
+    const newServer = Object.values(newServers)[0];


This line is as ugly as it gets.

I didn't want to change the interface of createServer (mainly due to laziness, probably).

There should be: a) a better [-reading] way to get the only server that's been returned; and/or b) a better interface for the function.

Well, you can't change the signature because it is an action creator, and the action getting dispatched is later passed to the reducer.

By changing the signature you would have to also adapt the reducer to support both use cases. So this is ok.

acelaya · 2021-06-07T18:39:30Z

src/servers/CreateServer.tsx


-    createServer({ ...serverData, id });
-    push(`/server/${id}`);
+    const newServer = Object.values(newServers)[0];


Well, you can't change the signature because it is an action creator, and the action getting dispatched is later passed to the reducer.

By changing the signature you would have to also adapt the reducer to support both use cases. So this is ok.

acelaya · 2021-06-07T18:40:01Z

src/servers/CreateServer.tsx


-    createServer({ ...serverData, id });
-    push(`/server/${id}`);
+    const newServer = Object.values(newServers)[0];


Suggested change

const newServer = Object.values(newServers)[0];

const [{ id }] = Object.values(newServers);

acelaya · 2021-06-07T18:40:19Z

src/servers/CreateServer.tsx

-    push(`/server/${id}`);
+    const newServer = Object.values(newServers)[0];
+
+    push(`/server/${newServer.id}`);


Suggested change

push(`/server/${newServer.id}`);

push(`/server/${id}`);

acelaya · 2021-06-07T18:51:33Z

src/servers/reducers/servers.ts

@@ -21,7 +21,7 @@ const serverWithId = (server: ServerWithId | ServerData): ServerWithId => {
    return server as ServerWithId;
  }

-  return assoc('id', uuid(), server);
+  return assoc('id', sha256(`${server.url}-${server.apiKey}`), server);


I have three main concerns with this approach.

The sha256 hash is sooooo loooong. It's quite overkill.

The API key is "leaked" in the URL, which is not very secure. Even less if we try to address previous point by switch to a shorter but also less secure hashing algorithm.

The hash is not unique, as a user could potentially set-up the same server more than once.

In order to address first two, I would instead use a different approach. Ideally, I would generate a slug of the title or the domain.

That way it's shorter, but also more human friendly and easier to spot which server are you opening. It also prevents the API key to be leaked in any way to the URL, even through a hash.

The third point is harder to address, but I think it's reasonable to append an index if the generated ID already exists, starting with 1. If it already exists, we increase it and try again.

When you create the servers from a servers.json file, the servers are processed in order, so the indexes should end up being deterministic.

In any other case, it will not be really a problem.

Thanks for the review. I meant to keep working on this PR last night, but here we are 😛

I though about leaking info in the URL, yes. But: a) sha256 hasn't been broken yet; b) whoever gets this URL (say we send it as an HTTP referer by mistake) already has access to the server's URL, which would publish the servers.json - so it's kind of game over for pre-defined servers anyway.

I know I'm biased to thinking about pre-defined servers (since my use case is to have a single server, and I want using it to be straightforward) so I'll trust your point of view more than mine with this. If you think a slug + index would work best - I can give it a try 👍

acelaya · 2021-06-07T18:52:09Z

src/servers/CreateServer.tsx

 import './CreateServer.scss';

 const SHOW_IMPORT_MSG_TIME = 4000;

 interface CreateServerProps extends RouterProps {
-  createServer: (server: ServerWithId) => void;
+  createServer: (server: ServerData) => { type: string, newServers: ServersMap};


Suggested change

createServer: (server: ServerData) => { type: string, newServers: ServersMap};

createServer: (server: ServerData) => CreateServersAction;

acelaya · 2021-08-21T15:54:03Z

Hey @matiasgarciaisaia. Do you plan to finish this PR?

matiasgarciaisaia · 2021-08-21T17:23:24Z

Hmmm... I'm not really sure.

Your suggestion is still to move to a slug + index solution?

acelaya · 2021-08-21T17:31:43Z

Yes

acelaya · 2021-08-22T07:07:59Z

@matiasgarciaisaia it's perfectly fine if you don't want to continue working on this for whatever reason. I just want to know to plan accordingly, and manage expectations from others that might think this feature is being worked on by someone.

matiasgarciaisaia · 2021-08-22T16:42:09Z

Seams reasonable.

Right now, I don't think I'll keep working on Shlink's web client for our use case at work. I might give this PR a try if I ever have an idle weekend, but I'm not doing too much of that now - so I'd say I may give this another try eventually, but probably don't count on that.

Use SHA256 to generate server IDs

c24b22c

To have stable, shareable server URLs, we use a hash of domain+ApiKey instead of a (local) UUID for each server. See shlinkio#435

matiasgarciaisaia commented Jun 7, 2021

View reviewed changes

acelaya requested changes Jun 7, 2021

View reviewed changes

acelaya closed this Aug 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SHA256 to generate server IDs #437

Use SHA256 to generate server IDs #437

matiasgarciaisaia commented Jun 7, 2021

github-actions bot commented Jun 7, 2021

matiasgarciaisaia Jun 7, 2021

acelaya Jun 7, 2021

acelaya Jun 7, 2021

acelaya Jun 7, 2021

acelaya Jun 7, 2021

acelaya Jun 7, 2021

matiasgarciaisaia Jun 7, 2021

acelaya Jun 7, 2021

acelaya commented Aug 21, 2021

matiasgarciaisaia commented Aug 21, 2021

acelaya commented Aug 21, 2021

acelaya commented Aug 22, 2021

matiasgarciaisaia commented Aug 22, 2021

	const newServer = Object.values(newServers)[0];
	const [{ id }] = Object.values(newServers);

	createServer: (server: ServerData) => { type: string, newServers: ServersMap};
	createServer: (server: ServerData) => CreateServersAction;

Use SHA256 to generate server IDs #437

Use SHA256 to generate server IDs #437

Conversation

matiasgarciaisaia commented Jun 7, 2021

github-actions bot commented Jun 7, 2021

Preview environment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Aug 21, 2021

matiasgarciaisaia commented Aug 21, 2021

acelaya commented Aug 21, 2021

acelaya commented Aug 22, 2021

matiasgarciaisaia commented Aug 22, 2021