Status code should be 500 on error #6893

Activity

1. blizzz

   added

   bug

   good first issueSmall tasks with clear documentation about how and in which place you need to fix things in.Small tasks with clear documentation about how and in which place you need to fix things in.

   on Oct 24, 2017
2. blizzz

   added this to the Nextcloud 13 milestone on Oct 24, 2017
3. Morikko commented on Oct 27, 2017

   

   Morikko

   on Oct 27, 2017

   Contributor

   More actions

   I can take this issue.
4. Morikko commented on Oct 27, 2017

   

   Morikko

   on Oct 27, 2017

   Contributor

   More actions

   The function OC_Util::checkServer that arrives after checkConfig also tracks if config is writable.

   • OC_Util::checkServer tracks config/ only
   • checkConfig tracks config/config.php

   Is it necessary to have those two checks ?

   Should I do a unit test for checking the header status ? If yes, in which test file should I add it ?
5. sophie-h commented on Oct 31, 2017

   

   sophie-h

   on Oct 31, 2017

   Author

   More actions

   I think it is completely sufficient to have an error when changing the configuration fails. I don't know if a proper handling in this case already exists.

   Having php files non-writable is a sane security decision for me. However, if writable config files are recommended, this information would fit into the "Admin" –> "Security & setup warnings" section.
6. tflidd commented on Nov 5, 2017

   

   tflidd

   on Nov 5, 2017

   Contributor

   More actions

   There is even an option for the config file in case the config file is read only:
   https://github.com/nextcloud/server/blob/master/config/config.sample.php#L648-L658

   Having php files non-writable is a sane security decision for me. However, if writable config files are recommended, this information would fit into the "Admin" –> "Security & setup warnings" section.

   I would prefer this option as well. Tell user to either set the option "read-only" in the config file or make it writable. Most people are quite responsive to messages in Security & setup warnings. A 500 status code error will trigger many users to end up with a broken setup and they will complain that the update procedure broke their system.
7. MorrisJobke

   added

   Hacktoberfest

   and removed

   Hacktoberfest

   on Nov 7, 2017
8. Morikko commented on Nov 17, 2017

   

   Morikko

   on Nov 17, 2017

   Contributor

   More actions

   Notes:

   • A first check is done in lib/base.php with function checkConfig(), this one is handle completely.
   • A second one in lib/private/legacy/util.php with function checkServer. This one only check the folder. Thus, it can throw an error if config folder is not writable but config file is writable. Moreover, the http status returned was already 503.

   Questions:

   • Should I change the http status returned with the second check to 500 ?
   • Should we open a new issue to correct the behavior of the second check to focus on the file instead of the folder ?
9. MorrisJobke

   added

   good first issue

   on Dec 29, 2017
10. rullzer

    removed this from the Nextcloud 13 milestone on Jan 10, 2018
11. MorrisJobke

    removed

    Hacktoberfest

    on Jan 12, 2018
12. jancborchardt

    removed

    good first issue

    on Jan 23, 2018
13. nextcloud-bot

    added

    staleTicket or PR with no recent activityTicket or PR with no recent activity

    on Jun 20, 2018
14. MorrisJobke

    added 2 commits that reference this issue on Jun 26, 2018

    Change status code when config is write protected

    ...

    b0f7796

    Change status code when config is write protected

    ...

    a34f70c
15. nextcloud-bot

    removed

    staleTicket or PR with no recent activityTicket or PR with no recent activity

    on Jun 26, 2018
16. MorrisJobke

    added this to the Nextcloud 14 milestone on Jun 26, 2018
17. MorrisJobke commented on Jun 26, 2018

    

    MorrisJobke

    on Jun 26, 2018

    Member

    More actions

    Should I change the http status returned with the second check to 500 ?

    This one is fixed in #9995 - inspired by @Morikko

    And @Morikko's PR was in #7205 - rebased as #9996
18. MorrisJobke

    closed this as completedin #9995on Jun 26, 2018