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

Staticman documentation enhancement #514

Merged
merged 9 commits into from Jul 8, 2019

Conversation

Projects
None yet
2 participants
@VincentTam
Copy link
Contributor

commented Jul 3, 2019

Webhook configuration

When moderation: true in staticman.yml, a PR containing a branch like staticman_5525bf80-055f-11e9-a156-295f601b09e8 is created. After merging such PR, such branch would become useless. We have better offer an advice for automatic deletion after merging to avoid such branches from piling up.

The technical details consist of several steps. I redirect interested users to the relevant page in the official doc, and adding necessary adaptations.

Changes in the comments in site config file

A sample Staticman API endpoint

The current endpoint parameter is borrowed from _includes/comments.html in mmistakes/minimal-mistakes#1845. Due to Staticman's URL scheme change introduced in v3, one has to add an extra parameter service which accepts either github or gitlab in the <form action="url">. Unluckily, this has not yet been documented in Staticman's official site. As a result, some users are not aware of such change, and spend time on fruitless try-and-error. Different blog themes might have different logic to complete the form action URL. For instance, in Hugo Swift Theme, in which I have been invited to collaborate, there's a gitProvider parameter. Nonetheless, I've no intention to change the template code in #440 to keep things working. According to Staticman's author's comment in eduardoboucas/staticman#264 (comment), Staticman's job is to get the HTML form data into the GitHub/GitLab repo, so the only appropriate place to document such theme-specific logic is the theme's README and relevant config files.

Little complement of #509

Sometimes, users might not read all of the section Staticman comments in the README. Therefore, a short reminder in the comments might catch them.

Updates in the comments in the repo config file

Background [TL;DR]

I received a 500 error during comment submission to the public API instance for Framagit (i.e. https://staticman-frama.herokuapp.com/) in a private clone of this repo. From the "server side", I know that I've got the correct API config parameters. It took me hours of experiments to rediscover the cause of such 500 error: at the bottom of staticman.yml, under the section reCAPTCHA, I've kept the default enabled: false for convenience, but I left secret unchanged while changing the endpoint in _config.yml. That reCAPTCHA secret was encrypted with this theme's default API instance (i.e. https://staticman3.herokuapp.com/), while the endpoint is holding another private RSA key. That would trigger a 500 error from the API server with a short and unhelpful response {success: false}.

Steps for reproducing a 500 error under this theme

  1. In config.yml: enable Staticman and change endpoint to another API instance.
  2. Keep reCAPTCHA settings at the bottom of staticman.yml (i.e. reCAPTCHA disabled, but with non-empty siteKey and secret.)

Since the endpoint is changed, the corresponding RSA private key no longer matches the one used for encrypting reCAPTCHA secret in the master branch. That would cause a 500 error from the API server, even if enabled is set to false, and reCAPTCHA parameters are empty in _config.yml.

Proposed solutions

Either one will do. @daattali please let me know which one you prefer. The README might need further editing before it can get merged.

Keep the reCAPTCHA section uncommented

In this case, no matter the user use reCAPTCHA or not, (s)he has to modify the repo config.

  • enable reCAPTCHA: supply own siteKey and secret
  • disable reCAPTCHA: change the values to siteKey: "" and secret: ""

I've written README based on this solution, but after creating this PR, I've found another simpler approach.

  # reCAPTCHA (OPTIONAL)
  # Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
  # Either use your OWN siteKey and secret, or change them to the empty string "".
  reCaptcha:
    enabled: false
    siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
    # (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
    # i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
    # For more information, https://staticman.net/docs/encryption
    secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw=="

Comment out the whole reCAPTCHA section

  • reCAPTCHA users can simply uncomment the parameters to get things work.
  • Otherwise, one can simply leave this untouched.

I personally prefer this approach. First, this is in line with the convention of switching off everything in this repo. Second, this requires less editing for non reCAPTCHA users. The only marginal drawback of this approach would be the additional difficulty to distinguish parameters from comments in the commented code below, but that shouldn't be an issue for normal users who know basic YML. If you prefer this approach, please let me know, so that I'll edit the README accordingly.

  # reCAPTCHA (OPTIONAL)
  # Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
  # Either use your OWN siteKey and secret, or change them to the empty string "".
  # reCaptcha:
    # enabled: false
    # siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
    # (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
    # i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
    # For more information, https://staticman.net/docs/encryption
    # secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw=="

@VincentTam VincentTam changed the title Add a link to webhook config for Staticman Staticman documentation enhancement Jul 3, 2019

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 4, 2019

I definitely prefer the second approach too. But what's the point of the enabled: false parameter under reCaptacha, I thought that should serve exactly this purpose, of turning recpatcha on/off. Can't we make it so that if enabled:false then everything else is ignored?

@VincentTam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

I definitely prefer the second approach too. But what's the point of the enabled: false parameter under reCaptacha, I thought that should serve exactly this purpose, of turning recpatcha on/off. Can't we make it so that if enabled:false then everything else is ignored?

Thanks for thoughtful feedback! My mind had blocked me. In between the two, there's a better way. I've just tested it at with 289412b..425de1c on my demo site: https://git.io/bjsm18. From VincentTam#24 created just after that, it's clear that your suggestion works.

  # reCAPTCHA (OPTIONAL)
  # Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
  # Use your OWN siteKey and secret
  reCaptcha:
    enabled: false
    # siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
    # (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
    # i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
    # For more information, https://staticman.net/docs/encryption
    # secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw=="

That's optimized for both parties.

  • the neccessary parameters catch our eyes at the first glance
  • reCAPTCHA users can simply uncomment and edit the parameters
  • non reCAPTCHA users can leave it as-in

I'm going to update README now.

@VincentTam
Copy link
Contributor Author

left a comment

Finished updating README and config files. @daattali please check. Despite reCAPTCHA is disabled by Staticman's default, it's good to leave these lines untouched.

  # Use your OWN siteKey and secret.
  reCaptcha:
    enabled: false

while commenting what's below in the template. In this way, reCAPTCHA's secret won't interfere with the Staticman API instance's RSA private key when reCAPTCHA is disabled.

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 5, 2019

Much cleaner! I would say that the parameters don't even need to be commented out though. Nothing will happen if they're left uncommented. Only params that must be ignored should be commented out

@VincentTam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Nothing will happen if they're left uncommented.

That's not true, and that's the whole point of starting this PR, after a day of testing this theme with another API instance (for Framagit). A blog theme's Staticman integration shouldn't care which particular Staticman API instance the site owner is using; Staticman shouldn't care whether GitHub Pages is used.

# reCAPTCHA (OPTIONAL)
# Register your domain at https://www.google.com/recaptcha/ and choose reCAPTCHA V2
# Use your OWN siteKey and secret.
reCaptcha:
enabled: false
# siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
# (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
# i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
# For more information, https://staticman.net/docs/encryption
# secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw=="

Even if enabled: false, if secret are either null or not encrypted with the RSA private key of the API instance defined in endpoint, then the Staticman API server will throw a 500 error. That's why there's a need to comment out recaptcha.siteKey and recaptcha.secret in our template staticman.yml, due to the convention to disable everything by default. Non-users of reCAPTCHA shouldn't get interfered with recaptcha.siteKey when they adopt another API endpoint.

You may see the template staticman.yml in another Hugo theme. There's a "" after each parameter to prevent getting a 500 error.

To sum up, the code block here is really the optimal template for both parties.

@VincentTam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

To make my observations more convincing, I've made this experiment.

  1. Cloned this project (GitHub allows at most one fork): https://github.com/VincentTam/BJPubTest at 2f4147c.

  2. Did minimal config at VincentTam/BJPubTest@042c7cf to set up GitHub project pages.

  3. Browsed the top post in the generated site: https://vincenttam.github.io/BJPubTest/2015-02-28-test-markdown/.

  4. Left a sample comment. Observed a 500 error.
    Screenshot from 2019-07-05 18-32-15

  5. Created a clone of this clone: https://github.com/VincentTam/BJPubTest1.

  6. Commented out the unused reCAPTCHA parameters at VincentTam/BJPubTest1@8162769 and invited @staticmanlab again.

    https://staticman-frama.herokuapp.com/v3/connect/github/VincentTam/BJPubTest1
    
  7. Browsed the top post in another generated site: https://vincenttam.github.io/BJPubTest1/2015-02-28-test-markdown/.

  8. Input the same text again. The same API server gives a 200 response, indicating a success.
    Screenshot from 2019-07-05 18-55-04

  9. Since moderation is disabled by default, the comment was pushed directly to the repo at VincentTam/BJPubTest1@8fd6508.

    # Whether entries need to be approved before they are published to the main
    # branch. If set to `true`, a pull request will be created for your approval.
    # Otherwise, entries will be published to the main branch automatically.
    moderation: false

Since "BJPubTest1" is a clone of "BJPubTest", they are identical except these two parameters.

repository in _config.yml VincentTam/BJPubTest VincentTam/BJPubTest1
recaptcha.secret in staticman.yml uncommented commented
result failed success

The room for improvement in my previous commit on Staticman repo config file is the whitespace on the right of the leftmost #.

reCaptcha:
enabled: false
# siteKey: "6Lcv8G8UAAAAAEqV1Y-XEPum00C_DxhD6O--qkFo"
# (!) ENCRYPT reCaptcha secret key using Staticman /encrypt endpoint
# i.e. https://staticman3.herokuapp.com/v3/encrypt/{your-site-secret}
# For more information, https://staticman.net/docs/encryption
# secret: "p5uHlH9hCqpMJaGKXdt5MEWFo7K6fX8hoYUwR3aIafOI6rtItLauaDCkGOucysJtrVZy+sHffioGzMsOU64JFDSyPQgrXujegcOHFRXHhD4fOUuBXSvV+OZ8JhSPTGWaRcQcoiGX4pT5hlebLddOl59b6sn6kU1ODQcEbhP83xVLZlaTWOrNrF5Wvy3TMXpH5gyl1tZEORxADAShMYyUbNR7XZYLEg1DfgIBHfIg3cKwdFt7KVLejFGKIiBYRAZDE2JuHItNmzJ2x9JgSK3E+XnShV5tuWpncnyFonJVHGEky/zRfUVLHobDMcJ/u9nlZqE8u47W+833F1WaIYuwNw=="

This can be deleted so as to be consistent with
branch: "master" # use "master" for user page
#branch: "gh-pages" # use "gh-pages" for project page


While constructing this example, I've made another observation that the link for invitation to the Staticman bot

https://{your-api}/v3/connect/{github|gitlab}/{user}/{repo}

and other Staticman-related parameters in _config.yml are case-sensitive. I'm going to add this to README and/or the _config.yml template so avoid errors from users' side.

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 6, 2019

Sorry if this is completely wrong, but I have to ask because it doesn't make sense to me. In my mind, if recaptcha is disabled, then the parameters for recaptcha shouldn't even be passed along - they should just be ignored. So I'm sorry if this question doesn't make sense, but:

What if in here, we add a check to only include these 2 parameters if recaptcha is enabled?

{% if site.staticman.reCaptcha.siteKey %}<input type="hidden" name="options[reCaptcha][siteKey]" value="{{ site.staticman.reCaptcha.siteKey }}">{% endif %}

@VincentTam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

What if in here, we add a check to only include these 2 parameters if recaptcha is enabled?

{% if site.staticman.reCaptcha.siteKey %}<input type="hidden" name="options[reCaptcha][siteKey]" value="{{ site.staticman.reCaptcha.siteKey }}">{% endif %}

You suggested modification won't change the result, because in BJPubTest (failed site)'s _config.yml, the two reCAPTCHA parameters are null.

# BJPubTest's _config.yml
  reCaptcha:
    siteKey  : # Use your own site key, you need to apply for one on Google
    secret   : # ENCRYPT your password by going to https://staticman3.herokuapp.com/v3/encrypt/<your-site-secret>

As a result,
Screenshot from 2019-07-06 09-32-59
in the failed example, the hidden fields doesn't contain options[reCaptcha][siteKey].
Screenshot from 2019-07-06 09-47-21

Sorry if this is completely wrong, but I have to ask because it doesn't make sense to me. In my mind, if recaptcha is disabled, then the parameters for recaptcha shouldn't even be passed along - they should just be ignored. So I'm sorry if this question doesn't make sense, but:

To make sense of my experiment, start thinking from the only factors changed in the two setups:

  1. repository in site config: necessary for sending HTTP POST requests to the right places.
  2. whether the two unused reCATPCHA comments are commented out in staticman.yml

Results: see the above table.

staticman.yml is Staticman config, which doesn't (and shouldn't) care whether the GitHub/GitLab repo uses GitHub/GitLab Pages or not, according to Staticman's author's comment in eduardoboucas/staticman#264 (comment). This config file is used by the API instance for communications between Staticman API server and GitHub/GitLab server. In the failed example, the two unused reCAPTHCA parameters are loaded; in the successful one, they aren't loaded.

Why your suggestion won't change the results?
As long as the input HTML form data structure matches the requirements in this repo config file, then Staticman will perform its work. As can be seen from the successful example (BJPubTest1), the HTTP POST request has the correct structure, so the API responsed correctly. Commit VincentTam/BJPubTest1@8162769 shows that the failed example (BJPubTest) is using exactly the same theme template, so the input HTML form data should have the correct structure.

What is the possible explanation?
The loaded unused parameters interfere the server-side communications even if reCAPTCHA is disabled in both _config.yml and staticman.yml.

My conclusion from this experiment is that the lack of # caused this error in my Framagit instance for the message "Hello world" with siteKey and secret unchanged. You're free to feel dubious about this, because you don't get such error during your testing with https://github.com/daattalitest/daattalitest.github.io. My explanation for your test results is that the default paramaters were tested with the default Staticman API instance, which holds a different RSA private key that was used for the encryption of the reCAPTCHA's site secret. You can test it with other Staticman instances by searching users "staticman" on GitHub. Another way to verify my conclustion is to break the reCAPTCHA site secret with strings like "123" while keeping enabled: false.

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 7, 2019

Thanks for all the detailed writeup explanations.

So if we go with the current minimal changes, would the user have to uncomment siteKey and secret when they set enabled:true ? If so, that should be mentioned in the yml file

Show resolved Hide resolved staticman.yml
Show resolved Hide resolved staticman.yml
Show resolved Hide resolved staticman.yml Outdated
Show resolved Hide resolved staticman.yml
@VincentTam
Copy link
Contributor Author

left a comment

Try using GitHub review.

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

The sample siteKey and secret - should they ever be used? Or are they only for demo purposes?

@VincentTam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

The sample siteKey and secret - should they ever be used? Or are they only for demo purposes?

These are my own siteKey and secret, so for others, they're only for display. That's why I've emphasized "OWN" in the Staticman config file.

List of domains hooked up with my reCAPTCHA v2 site key
Figure: List of domains hooked up with my reCAPTCHA v2 site key

daattali added some commits Jul 8, 2019

@daattali daattali merged commit 94161db into daattali:master Jul 8, 2019

@daattali

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

Thanks Vincent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.