Skip to content
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.

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

add token to LinkAccessedEvent #6554

Conversation

dragonchaser
Copy link
Member

We added the link token to the LinkAccessedEvent

refs #3753

Signed-off-by: Christian Richter <[email protected]>
@dragonchaser dragonchaser force-pushed the issue-3753-issues-with-audit-public_link_token_in_event branch from 03e2958 to 14a66a9 Compare June 19, 2023 11:53
@dragonchaser dragonchaser requested review from kobergj and removed request for kobergj June 19, 2023 11:58
@dragonchaser dragonchaser mentioned this pull request Jun 19, 2023
9 tasks
Signed-off-by: Christian Richter <[email protected]>
@dragonchaser dragonchaser force-pushed the issue-3753-issues-with-audit-public_link_token_in_event branch from e33b984 to 6d9ad6f Compare June 19, 2023 12:45
@sonarcloud
Copy link

sonarcloud bot commented Jun 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@dragonchaser dragonchaser marked this pull request as ready for review June 19, 2023 12:58
@kobergj
Copy link
Collaborator

kobergj commented Jun 19, 2023

Merging this will allow the sysadmin to access all files that have a public link. In my opinion this is a major security issue. We should not log the token at all, even if it means we don't know which link was being accessed.

@butonic @micbar @tbsbdr

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Code looks good. Just blocking until above comment is resolved

@butonic
Copy link
Member

butonic commented Jun 20, 2023

Merging this will allow the sysadmin to access all files that have a public link. In my opinion this is a major security issue. We should not log the token at all, even if it means we don't know which link was being accessed.

The public link token is part of the url and will already show up in access logs anyway. So, I understand your concern, but I don't see a decrease in security. If users don't want their public links to be accessed ... set a password.

@kobergj
Copy link
Collaborator

kobergj commented Jun 20, 2023

IMO we should not log the token at all. And already having a security issue doesn't mean we should open another one.

I also don't see the point in having the token logged. What is the admin supposed to do with that information? They would need to know whose link it is to be able to do any action.

@dragonchaser
Copy link
Member Author

IMO we should not log the token at all. And already having a security issue doesn't mean we should open another one.

I also don't see the point in having the token logged. What is the admin supposed to do with that information? They would need to know whose link it is to be able to do any action.

Seeing tokens in the access log is not a security issue per se, works as designed....

@kobergj
Copy link
Collaborator

kobergj commented Jun 20, 2023

Seeing tokens in the access log is not a security issue per se, works as designed....

Not so sure about that, smells like bug to me.

Anyways the access log is not part of this PR. So we should leave that out of discussion. Logging the access tokens in one place doesn't make it better to log them in another.

@dragonchaser
Copy link
Member Author

I think this is a decision pm should make... @kobergj I see and understand your concerns, but this is more of a general decision how this should be handled.

/cc @tbsbdr

@kobergj
Copy link
Collaborator

kobergj commented Jun 20, 2023

I wouldn't say this is a general decision. Opening backdoors should always be a case to case decision.

But I agree. We need a product decision.

/cc @micbar

@micbar
Copy link
Contributor

micbar commented Jun 20, 2023

From my pov, this is not critical.

We need to remember that "Public Links" are designed to be "public".

@dragonchaser
Copy link
Member Author

@micbar so shall we merge this now?

@micbar
Copy link
Contributor

micbar commented Jun 20, 2023

Yes.

@dragonchaser dragonchaser merged commit 961b03b into owncloud:master Jun 20, 2023
2 checks passed
@dragonchaser dragonchaser deleted the issue-3753-issues-with-audit-public_link_token_in_event branch June 20, 2023 11:33
ownclouders pushed a commit that referenced this pull request Jun 20, 2023
…it-public_link_token_in_event

add token to LinkAccessedEvent
@micbar micbar mentioned this pull request Jul 24, 2023
68 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants