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

Reviewed Security In Piwik #36

Merged
merged 2 commits into from
Nov 26, 2014
Merged

Reviewed Security In Piwik #36

merged 2 commits into from
Nov 26, 2014

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Nov 22, 2014

Related to #22

I'm opening this as a PR because it depends on #34 to be merged (else code blocks will not work).

Not many changes but the diff seems to be a bit messed up.

Regarding Preventing Direct Access, is that common that plugins provide files that execute something? Because if that's not common/shoudn't happen, then I'd rather replace that part with something like this:

All the PHP code provided in plugins should declare PHP classes (or functions or constants). Running that code should have no side effect (e.g. should not execute anything).

E.g. in PSR-1 there's this rule:

Files SHOULD either declare symbols (classes, functions, constants, etc.) or cause side-effects (e.g. generate output, change .ini settings, etc.) but SHOULD NOT do both.

I'd say code causing side-effect should not exist in plugins. Does that makes sense?


$rows = Db::query($sql, array($idSite));
$rows = Db::query($sql, array($idSite));
```

There is a limit to the number of placeholders you can use. If you need to use more placeholders than the limit allows, you may have to concatenate the parameters directly. Make sure these parameters are obtained from a trusted source (such as from another query).
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph and the one below, I cannot understand it. Could we remove them?

@mattab
Copy link
Member

mattab commented Nov 25, 2014

I'd say code causing side-effect should not exist in plugins.

+1 - this part of the guide was really old and outdated...

Feel free to merge after those!

@mnapoli
Copy link
Contributor Author

mnapoli commented Nov 26, 2014

All done

mnapoli added a commit that referenced this pull request Nov 26, 2014
Reviewed Security In Piwik
@mnapoli mnapoli merged commit 2b7458d into master Nov 26, 2014
@mnapoli mnapoli deleted the security branch November 26, 2014 00:25
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.

2 participants