-
Notifications
You must be signed in to change notification settings - Fork 180
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
Add Craft CMS fingerprinter #434
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @OccamsXor, thanks for your contribution!
I'm reviewing the fingerprinter and I've found some issues with it. I've already left some comments, but there are a few things which are a bit difficult to write in review comments, so here they are:
About the different files for version 3 and 4:
- Separating the environment variables in two files can be avoided, you can just put all the environment variables for versions 3 and 4 in the same environment file and each version will pick up the ones they use and ignore the others
- As a result, you can avoid having two different env files and two different versions.txt files
About the update.sh
file:
The CreateFingerprintForCraftCMS
function has several issues:
- Uses
curl
andunzip
without checking if the commands are available, there's no guarantee that they are installed - The
chown
command can only be run as root - The entire procedure should really be done inside the container
Assuming that you join the contents of the two .env
files in a single file called env.txt
and the two versions files in a single file called versions.txt
, here's an example on how to implement the Docker image correctly:
You can create a Dockefile
that adds the required files at build time and sets the permissions correctly:
FROM craftcms/nginx:8.2-dev
ARG CRAFT_VERSION
USER root
RUN mkdir -p /app
WORKDIR /app
RUN wget "https://github.com/craftcms/cms/releases/download/${CRAFT_VERSION}/CraftCMS-${CRAFT_VERSION}.zip" -O CraftCMS.zip
RUN unzip -o CraftCMS.zip
COPY env.txt /app/.env
RUN chown -R www-data.www-data /app
USER www-data
Then modify the docker-compose.yml
file to use the local Dockerfile instead of the craftcms image, passing the craftcms version from environment variables:
web:
build:
context: .
args:
CRAFT_VERSION: "${CRAFT_VERSION}"
ports:
- 8080:8080
...
This way, the update.sh
file can be greatly simplified, as the docker compose file can be used directly, without copying files around:
StartCraftCMS() {
local version="$1"
pushd "${APP_PATH}" >/dev/null
CRAFT_VERSION="$version" docker compose up --build --wait -d
docker exec -it craftcms-web-1 php craft install/craft --email [email protected] --username admin --password tsunami --site-name local --site-url http://localhost:8080 --language en-us
popd >/dev/null
}
StopCraftCMS() {
pushd "${APP_PATH}" >/dev/null
docker compose down --volumes --remove-orphans
popd >/dev/null
}
CreateFingerprintForCraftCMS(){
local version="$1"
StartCraftCMS "$version"
checkOutRepo "${GIT_REPO}" "${version}"
RESOURCES_PATH="${GIT_REPO}"
updateFingerprint \
"craftcms" \
"${version}" \
"${FINGERPRINTS_PATH}" \
"${RESOURCES_PATH}" \
"http://localhost:8080"
StopCraftCMS
}
If you want you can just copy this code, I've verified that everything works as expected.
Also, please add the latest versions of CraftCMS to the version file.
google/fingerprinters/web/scripts/updater/community/craftcms/update.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the file to not be hidden (without the starting dot .
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the file to not be hidden (without the starting dot .
)
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Outdated
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Outdated
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Outdated
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Outdated
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/app/docker-compose.yml
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/update.sh
Outdated
Show resolved
Hide resolved
google/fingerprinters/web/scripts/updater/community/craftcms/update.sh
Outdated
Show resolved
Hide resolved
Hi @OccamsXor, are you still interested in contributing to Tsunami with this plugin? |
@OccamsXor Ok, please start by applying the suggestions, as it may be enough to merge this. If more complex modifications are needed I'll let you know or I'll try to put them as suggestions so that you can easily apply them using the web interface. |
…pdate.sh Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pp/docker-compose.yml Co-authored-by: Savio Sisco <[email protected]>
…pdate.sh Co-authored-by: Savio Sisco <[email protected]>
…pdate.sh Co-authored-by: Savio Sisco <[email protected]>
Co-authored-by: Savio Sisco <[email protected]>
Hey @tooryx @maoning,
Web application fingerprint for Craft CMS.
This PR includes the update script, docker-compose.yml, environment files, version files, and binproto file. I used two different environment and version files because they are required for different Craft CMS versions.
This is my first contribution to the project. Please let me know if there are any changes I should make.
This resolves #406