Reader: Logged out liking is broken

[artemiomorales]

When liking a post, if one is prompted to create an account, the like isn't reflected after completing the signup flow and returning to the Reader.

[davemart-in]

We should test this again. I sort of wonder if this is replicable, or if we ran into this because we were trying to test through BrowserStack.

[Addison-Stavlo]

I sort of wonder if this is replicable

I tested on my end and can repro. The like wasn't in place once being logged in and wasn't there after reloading either.

[davemart-in]

Thanks for double checking. We should probably prioritize this issue in that case. Changing it to critical.

[holdercp]

@artemiomorales Just to make sure I know the flow.

1. We're logged out
2. We go to an unauthenticated Reader page, like /discover
3. I like a post in the feed
4. I'm prompted to either log in or create an account in a new browser window
5. Once I do either (log in or create an account) and I'm returned to /discover, my like has not persisted

Is this the flow?

[artemiomorales]

Is this the flow?

Yup! That's correct.

[holdercp]

@davemart-in Looks like this isn't a bug is something that was intended to be implemented as a v2 to this project: pe7F0s-1gz-p2. See this comment specifically: pe7F0s-1gz-p2#comment-1634.

Happy to create issues/PT for it, or just go ahead an work on implementing it, but I wanted to make sure this was prioritized correctly.

[davemart-in]

Let's just use this as the issue.

@holdercp Happy for you to do a one day spike to see if you can either A) solve the issue, or B) better understand how long it will take to fix.

[Addison-Stavlo]

do a one day spike to see if you can either A) solve the issue, or B) better understand how long it will take to fix.

I took a look today and hacked up a solution leveraging local storage. #102672 (I will document this PR more tomorrow). This ensures the actions get saved and dispatched for the user upon logging in.

There are still some race condition inconsistencies to look into and address: sometimes when you log in you see the like and other times it requires another reload to appear - this is likely due to a race condition between the like action we are dispatching and the request for posts/likes going out at the same time on that initial load. There are likely similar race condition issues with the follow tag and subscribe actions to look into as well. Im not yet sure if it makes sense to look at all these separately in follow up, and how much they may more may not expand the scope.

---

There are some minor concerns / potential for mild bug or abuse with this approach I will discuss below. They seem pretty limited, but worth noting:

On the intended abuse side, someone could manually or through an insecure browser extension set the local storage causing the next person that logs in (or next reload) to dispatch that action. However, the action would be limited to the items we choose to allow: items such as liking posts/comments, following a tag, or subscribing to a site (side note, supporting subscribing from logged out would always be inconsistent since these require email verification and not all signup methods come verified by default). In general the potential seems low, but if a user is installing malicious non-trusted browser extensions and granting them wide permissions that could be an avenue for abuse. However, that abuse would be limited to 1 supported action per full reload / login.

Aside from abuse, there is a small bug potential i have taken a step to help mitigate: if a user were to bail on the login flow after triggering the pending logged out action, the next user to sign in would then have that action dispatched. I have limited the potential for this by adding a date time on the pending action and only using it if it is less than 5 minutes old, so there is also minimal risk for running into this type of bug.

Some other notes about other approaches that didn't seem viable:

• using withPersistence to have the redux store save the state between reloads doesn't work for us in this case since that is user specific and we are going from logged out to logged in states.
• Separately, trying to convey the action in query params in the login modal's redirect url upon login would create a coverage gap since that is never triggered for accounts that have to sign in with email links. Similarly, waiting to set the local storage until this modal sees the login success would have the same gap in coverage.

[davemart-in]

There are still some race condition inconsistencies to look into and address: sometimes when you log in you see the like and other times it requires another reload to appear - this is likely due to a race condition between the like action we are dispatching and the request for posts/likes going out at the same time on that initial load.

This is probably too hacky, but could we add a 500ms timeout?

On the intended abuse side, someone could manually or through an insecure browser extension set the local storage causing the next person that logs in (or next reload) to dispatch that action.

Could we add a server generated nonce check to help prevent this?

if a user were to bail on the login flow after triggering the pending logged out action, the next user to sign in would then have that action dispatched. I have limited the potential for this by adding a date time on the pending action and only using it if it is less than 5 minutes old, so there is also minimal risk for running into this type of bug.

The only other thing that comes to mind would be to add the user ID to PENDING_ACTION_STORAGE_KEY as well and check it to make sure we have the same user.

[Addison-Stavlo]

This is probably too hacky, but could we add a 500ms timeout?

I was thinking about something like that as well. It could help reduce the chance but not resolve it completely. Im unsure how effective it would be given some of the initial load times that can happen 🤔. And depending on location and way requests get routed, it could be more effective in some regions than others. Another thought could be to trigger a modal with a loading indicator and message that forces another reload after a couple seconds, ensuring that next request for posts/likes is sent after the like has been established. While extra reloads are not generally a great experience, if its only happens through this login flow maybe that is acceptable.

The only other thing that comes to mind would be to add the user ID to PENDING_ACTION_STORAGE_KEY as well and check it to make sure we have the same user.

We are going from logged out to logged in, there is no user ID logged out and not really a way to compare any user credentials to verify the person that set the pendingAction while logged out is the same that signs in. I think gauging by how long ago that pendingAction was set might be the closest indication we can get. 5 minutes seems reasonable, but we could even reduce it if we think that is too long.

Could we add a server generated nonce check to help prevent this?

For similar reasons Im not sure this would do much. If generating it logged out it would have to not require authentication, but then couldn't be tied to the logged in user specifically. The malicious extension could theoretically still just get this just as easily retrieve and set it. It seems more like a false sense of security. In all I think the risk is very limited here in that:

• This is limited to basic and reversible actions: liking/following.
• The dispatch of this is limited to once per login/full reload.

More broadly speaking, if a user had a malicious extension with too many permissions there are much worse concerns and things it could do. It could essentially have full control over a logged in session anyways. So after thinking about this more, tightening down the security around this specific logged-out to logged in pending action handoff would feel a bit moot, as the same malicious extension that could stage that would be able to have full control while logged in anyways sending whatever requests or injecting whatever JS it wanted. This isn't a flaw with our app, but the reality of extensions and why it is not recommended to install those from untrusted sources and why modern browsers require you to explicitly approve permissions. Anyways, that all said, I don't think we are really adding any more potential for abuse than already exists.