All images

@nwalters512 as mentioned in the meeting, I added (in 9f87ae0) the ability for instructors to set a dynamic file name as absolute based on the workspace home directory. Can you please review that portion to make sure it looks fine?

@jonatanschroeder for posterity, can you comment here stating why that change is being made? What's the instructor use case?

I thought about this a bit more and I have vague worries about how that would behave if an instructor changed the home directory for a question, and then a student reset their workspace. That's probably unlikely to happen in practice, but that's the kind of thing that would have worked if absolute paths to the home directory weren't used.

@jonatanschroeder for posterity, can you comment here stating why that change is being made? What's the instructor use case?

I thought about this a bit more and I have vague worries about how that would behave if an instructor changed the home directory for a question, and then a student reset their workspace. That's probably unlikely to happen in practice, but that's the kind of thing that would have worked if absolute paths to the home directory weren't used.

That is a good point. In essence, depending on the question's functionality, some interesting patterns may arise in terms of file name structure. So these files should be accepted:

• file_name.txt (i.e., no path);
• subdir/file_name.txt (with a path, but all within the home directory);
• subdirA/../subdirB/file_name.txt (while unconventional, depending on the complexity of the generate process, I see no reason to deny it).

While these files should be denied, mostly because they are not mapped in the volume and should thus be explicitly listed to instructors as an error, but also to avoid problems trying to save these files:

• ../file_name.txt;
• /etc/file_name.txt (absolute path that has nothing to do with the home directory);
• subdirA/../../file_name.txt (resolution goes outside the home directory).

The handling of the files above was supposed to cause an issue, but the current implementation in master causes a weird behaviour that allows it to proceed, mostly because the contains function expects the parent directory to be an absolute path, while the function was sending a relative path. This was fixed in the current implementation.

Now this leaves some fuzzy options. Assuming that the workspace home directory is /home/prairie, what should be done with directories like these?

• ../prairie/file_name.txt (navigates outside the home directory, but resolves inside);
• /home/prairie/file_name.txt (absolute path that includes the home directory itself).

The current implementation allows both to proceed, but I'm perfectly happy to also change it so they are both denied. I don't see a strong reason in either direction. Allowing them would not cause a significant add of functionality and the corresponding functionality is easily replaced with a similar alternative, while the rationale for denying it would only cause a problem if there is a change in home directory that does not align with the code in generate, but changing the home directory would typically also rely on corresponding changes in the workspace itself, so it's unlikely to happen often.

One alternative implementation to deny the fuzzy options above would be to replace path.resolve with path.normalize, and then deny if path.isAbsolute or if the normalized path starts with ... This could be implemented as a special case of contains if the parent directory is null.

I would lean towards being as restrictive as possible by default, which will also push people towards doing what is probably the most correct thing. That is: their paths should always be relative to whatever the home directory might be. Your suggestion to reject absolute paths entirely sounds like a good one.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.30%. Comparing base (c5ecd8a) to head (27572ce).

@@            Coverage Diff             @@
##           master    #9855      +/-   ##
==========================================
+ Coverage   67.00%   67.30%   +0.30%     
==========================================
  Files         456      456              
  Lines       71318    71363      +45     
  Branches     5714     5751      +37     
==========================================
+ Hits        47787    48033     +246     
+ Misses      23102    22903     -199     
+ Partials      429      427       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I would lean towards being as restrictive as possible by default, which will also push people towards doing what is probably the most correct thing. That is: their paths should always be relative to whatever the home directory might be. Your suggestion to reject absolute paths entirely sounds like a good one.

Done in 1ab3637. I've extended the functionality of the path-utils package. However I tried to add a changeset to it but it didn't list path-utils as an option. Is this expected?

path-utils is a private package, meaning it won't ever be published to npm:

Since it's not published, it doesn't enforce a changelog.

Thanks for making the changes, I'll review them next week!

@nwalters512 just making sure this doesn't fall through the cracks, I'm looking for a review of the changes done after the approval above.