-
Notifications
You must be signed in to change notification settings - Fork 299
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
Users with only acl:Append permission are able to POST ACL files #1418
Comments
It is only "considered as a valid ACL" because of the automatic relation with a URI template that's in use (which happens to match the created resource) ie. id acl id.acl. If bar.ttl exists and has rel=acl to bar.ttl.acl, I agree that this is an NSS bug and it should reject the request to create bar.ttl.acl from user with acl:Append - if it didn't do anything about the Slug suggestion. If it did, then it'll just rename to something "safe". If however bar.ttl doesn't exist, bar.ttl.acl should be handled as any RDF source - not to be "considered as a valid ACL" unless of course if NSS reserves .acl suffix in resource names for "valid ACL"s. So, it shouldn't allow bar.ttl.acl to be created where it can potentially be linked from bar.ttl. |
I don't think it should be handled as any RDF source. The association of a given resource R to an acl resource A in NSS is based on the presence of '.acl' at the end of a A's name, when the part before the .acl in A's name is the same name as R. If A's name is just '.acl', it would apply to the container that A is a member of. Consider the creation of a resource named 'foo' in container C. If NSS doesn't ensure that there isn't a foo.acl in C first, anyone that can predict that a file named foo will be created at some point can set the permissions in advance by making foo.acl, even if they don't have control permission on container C. IMO - the simplest path would be for NSS to reserve the .acl suffix to avoid this. Note this is an NSS problem, not a Solid problem. In Solid you can make any resource names you want, and the server maintains the association of acls. It is how NSS maintains that association that leads to this potential issue. |
|
Hi Guys, by default, I have CONTROL to the folder & want to add Authenticated Agents as Submitter (https://spoggy-test.solid.community/public/shighl_test/inbox/) Here is the code I use
|
It would be relevant to know what content-type you specified in the PUT request that generated the 500 error. |
@jeff-zucker
That worked yesterday. Can you create an .acl file today with solid-file-client on a solid.community pod? |
Can you try to do it manually using dataBrowser. Your acl is wrong You are also missing in Read block |
@bourgeoa sure if I want that the inbox can be read by everyone, i need But my question is does this fix could have any influence to the @jeffz solid-file-client putting .acl or not ? As I said there was no matter yesterday to put that file... Is there a server where that patch has not been applied so i could test ? |
@jeffz here you can test how I try to create .acl with solid-file-client https://scenaristeur.github.io/agora/acl_test.html |
confirmed by the same test on @bourgeoa server that did not apply the last patch . |
confirmed it doing a manual PUT
|
@jaxoncreed it('user2 should not be able able to post an acl file', function (done) {
var options = createOptions('/append-acl/', 'user2', 'text/turtle')
options.headers.Slug = 'some.acl'
options.body = '<a> <b> <c> .\n'
request.post(options, function (error, response, body) {
assert.equal(error, null)
assert.equal(response.statusCode, 403)
assert.equal(response.statusMessage, 'User Unauthorized')
done()
})
}) |
I set up tests in my fork that disclose the described behaviour properly (based on the version before the "fix") https://github.com/angelo-v/node-solid-server/pull/2/files |
@scenaristeur this bug has nothing to do with solid-file-client, it is an NSS bug that occurs regardless of what library does the file writing. When you encounter a problem, you should try to break it down to its simplest form, removing all intermediary software. In this case some plain PUT and POST tests as @angelo-v showed rather than using solid-file-client to write. This helps people identify where the problem is, makes tests easier to reproduce, and saves me from having to check solid-file-client every time there is an NSS bug. If you find that a straight PUT or POST behaves correctly and solid-file-client does not, that's when you should file an issue on solid-file-client. If you need help writing plain PUT and POST tests, let me know and I'll be glad to give you some tips. |
@jeffz sorry for disturbing you if the problem is not on solid-file-client client, but I first wanted to share that there I encountered a problem and asked if that patch was the cause. I shared the way / context I encountered it and made a reproductible test. As a sfc expert you could tell me if I was wrong or not. If I was not wrong, the problem could be in sfc or somewhere else, but I personally didn't know where. Then @bourgeoa give me access to a non patched provider and we confirmed that incompatibility between the new patch and sfc. Then @bourgeoa & @angelo-v excluded the sfc responsability . During 2 days I've tried to alert about a problem and explain as I could understand the cause... and I thought that if I encountered that error, some other user of sfc could encounter it. If that patch was applied, it would break some sfc capabilities and that's why I wanted to alert you personally. But I'm not a expert in .Acl, authorization, post,put,patch, system... Really sorry for disturbing you. If you prefer, the next time it happens, I say nothing the patch is applied, and sfc stay broken until someone, one day try to put an Acl file with sfc. |
David, I'm not asking you to stop notifying me if there are problems or asking for my help in tracking them down. You are always welcome to do that. I am asking that when you report a problem, you reduce the problem to the least number of elements and test those. |
That's what I did with my knowledge 😊. |
@jeff-zucker I feel your pain. I had to take two days out, including my birthday, for this minor patch too. I feel very disappointed that node solid server would release this as a patch release and it causes disruption in both the server and apps I'm not beating up on @jaxoncreed here either, he's made it clear he's over loaded and that he contributed a patch and it passed the tests was amazing Also both @jeff-zucker @scenaristeur and @bourgeoa did nothing wrong here Inrupt was created with a clear mandate to fix the bugs in the reference implementation. Everyone was on the same page with that. We need jackson to be given the resources he needs to do this. For a proper review (there was none). A proper mapping out of the public API and noting the change. HTTP 500 should not occur in any case, and given that node is single threaded, this could be a cause of the long time outs we have seen (still unclear). Changes in the pubic API require a semver minor version bump. Having a working reference implementation is always more important than branding Room for improvement @timbl @johnbizguy Edit: corrected and linked to semver spec, added the http 500 note, and time outs, more constructive text |
For me, there is no question of "doing wrong". I have no doubt @scenaristeur was trying to help and that @jaxoncreed is over-worked. Agreed that more notice and testing time for server changes would help, but as you noted, that takes resources. |
@melvincarvalho Happy Birthday 🎂🥂 Thank you all... Now I can PUT some .Acl file with sfc 😊. |
Thank you to Melvin and others for taking time to express concern and send caution. As always we encourage everyone to post feedback and suggestions — a great example of how Solid benefits from the multiple eyes of an open source community. When the Inrupt team was notified of an important security flaw we responded as fast as possible to provide a fix; to help as quickly as possible keep data protected. Unfortunately as sometimes happens a rapid response test process was incomplete and broader functionality in the ecosystem was impacted by closing this flaw. We’re now taking action to achieve more thorough testing procedures before we submit even the most critical fixes to the community for review. We are appreciative of the contributions from this group and others to Solid — The many innovative applications shown off during Solid World a few weeks ago were very encouraging and we’re pushing to get this flaw resolved ASAP to help get everyone back on track. |
@scenaristeur turned out to be right about this and I was wrong. Solid-file-client was part of the problem here. As I understand it, @jaxoncreed corrected @angelo-v 's original problem of the slug on POST being ignored when posting a .acl allowing those without Control to post .acls. That fix triggered @scenaristeur's issue of 500 on PUT of a .acl because solid-file-client has, in recent versions, been sending a slug with PUT. @jaxoncreed rapidly fixed things on the server end (thanks!) and we will also remove the slug from PUT on the solid-file-client end. Thanks to all for a rapid resolution. |
No @jeff-zucker NSS is broken. Yes, sending the Slug with PUT is not correct, but still here is work to do as my tests indicate, see https://github.com/angelo-v/node-solid-server/pull/2
As I understand it currently (still not sure, would need more time to investigate), the changes do not fix the problem, they break just enough of the server to make the bug go away as well and replace it with a 500 response. |
@angelo-v did you see @bourgeoa 's last comment at #1422 (comment) seeming to indicate that the fix works. Not sure if he tried your test though. |
Thanks @jeff-zucker I was not aware of this new PR, which looks fine. I proposed to add the tests I wrote to make sure it works as expected. |
Hey @scenaristeur, thanks for your patience on this. Your issue really illuminated some spec clarification that you can see talked about more here We think the best way to proceed is to no longer allow ACLs to be created with POST and a Slug header, but there are other ways you can update your app to create ACLs. The best example is in the react SDK code. Creating an ACL for a resource or a container comes in 2 steps: Step one: read the resource header to determine the specified location of the acl:
Step two: send a PUT request to the aclUrl
Hopefully, that helps out, and sorry for all the confusion. |
Or, to accomplish the same thing with solid-file-client : let response = await fc.getItemLinks( resourceURI )
await fc.createFile( response.acl, ACLcontent, "text/turtle" ) The first call finds the link information in the resource's header, the second uses PUT to write content to it. |
Woh Sorry for all the Brainstorming it causes... But I promise you, Now I'll use something to get Acl Link before posting a new one ;-) |
I suppose the issue can be closed |
Actual behaviour
Given a container where I have
acl:Append
permission, but noacl:Control
permissionWhen I POST a file to the container using a Slug header ending with
.acl
Then the resource gets created and is considered as a valid ACL
Expected behaviour
Creating ACL resources is only possible with
acl:Control
permissionExample
Container at path
/foo
with public write permission containing a resourcebar.ttl
Request to create an ACL file for
bar.ttl
:The text was updated successfully, but these errors were encountered: