Skip to content

Conversation

@mschwager
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 74.86% when pulling b6eb2d1 on mschwager:master into f4bc6c8 on mebjas:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 74.86% when pulling b6eb2d1 on mschwager:master into f4bc6c8 on mebjas:master.

@mebjas
Copy link
Owner

mebjas commented May 1, 2015

@mschwager Thanks for the pull, I'll review and merge ASAP!

Copy link
Owner

Choose a reason for hiding this comment

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

Normally when the config file is changed the main js file is automatically changed, that's what self::createNewJsCache(); LINE#202 does. Now if due to file permission error it fails, we had to insert the urls inline so that the system doesn't fail.
So insert this hidden field only when say count($urls) > 0. Also JS should try to retrieve urls from the hidden field only when the hidden field is available.
Initiating the init() on onload event is good, and we need to be compliant with CSP rules.

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.12%) to 63.54% when pulling 0cb5bef on mschwager:master into f4bc6c8 on mebjas:master.

@mebjas
Copy link
Owner

mebjas commented May 4, 2015

Currently how it works is, when ever the config file is changed, the library creates a new version of js code (libs/csrf/csrfpJsFileBase.php is the base JS code, using which the code is generated).
The reason for doing this are:

  • It allows passage of Allowed URL to javascript code
  • It gives the flexibility for library users to customise the token name according to need, so that it doesn't clash with any of their own identifier name.

Now putting these data to HTML code and picking them from JS code seems a nice idea but we'd have to hardcode identifiers of those hidden fields which clashes with second advantage we had in current method. But this removes other overhead -
which is in case the library doesn't have write access to the js code, then for every request the lib attempts to create the file, catches an exception and inserts a static code to output HTML.

So now we have lot of options:
OPTION 1 Keep current version, because overhead is smaller in front of flexibility.
OPTION 2 Keep current version and make it mandatory for users to provide write access to lib, thus no overhead, + flexibility of using any identifier name for lib user.
OPTION 3 Use the method of inserting these data to HTML as hidden elements and pick them from JS, hardcode a super unique identifier for them.

@abiusx how about a suggestion?
@mschwager thanks for pointing this thing to me, OPTION 3 seems best to me!

@mebjas
Copy link
Owner

mebjas commented May 7, 2015

@mschwager I have merged your pull to a maser-dev branch. I have made further changes and soon push it to master.
9b9e40d
Thanks!

@mebjas mebjas merged commit 0cb5bef into mebjas:master May 7, 2015
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.

3 participants